- This was to make implementing jthread easier and requested in https://reviews.llvm.org/D151559
Details
- Reviewers
Mordante ldionne - Group Reviewers
Restricted Project - Commits
- rGcea4285949b5: [libc++][NFC] Granularise <thread> header
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM after undoing the formatting changes and the CI passes.
Can you make two followup commits
- format all the new files, I don't like to do that is the same commit, that makes tracking changes in git harder
- modernize the code _LIBCPP_INLINE_VISIBILITY -> _LIBCPP_HIDE_FROM_ABI, _VSTD -> std, typedef -> using
libcxx/include/thread | ||
---|---|---|
117–119 | Please don't mix formatting and splitting in one patch. |
LGTM w/ a few comments and green CI.
libcxx/include/__thread/formatter.h | ||
---|---|---|
31–33 | I think I would only keep this error message in the top-level <thread> header. That's just for consistency with e.g. <filesystem>. (here and everywhere else) | |
libcxx/include/thread | ||
93–95 | Let's remove those includes since they don't contribute to the public API of <thread>. At first we tried to systematically include all headers from <__foo/*.h> inside <foo>, but we noticed it didn't work (I think it was causing circular dependency issues but I think @Mordante might remember more context) so now I guess the only consistent thing we can do is include only the ones that contribute to the public API of <foo>. | |
100–102 | Let's fix those tests instead. |
libcxx/include/thread | ||
---|---|---|
93–95 | I don't exactly recall. I thought still wanted to include all <__foo/*.h>. That might be less important nowadays since most headers use the granularized parts of a header instead of the entire header. |
libcxx/include/thread | ||
---|---|---|
117–119 | Why are these headers only removed when explicitly setting _LIBCPP_REMOVE_TRANSITIVE_INCLUDES? |
I think I would only keep this error message in the top-level <thread> header. That's just for consistency with e.g. <filesystem>. (here and everywhere else)