A few __fwd includes are missing from public modules that will become noticeable when the private submodules are split into their own top level modules (D144322). Add the missing includes.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/algorithm | ||
---|---|---|
1746 | Thanks for fixing this. The comment below isn't about your change in particular, but rather the general structure libc++ has invented for itself recently. I'm concerned because these are potentially fairly serious bugs, and they would never had occurred had the things in algorithm been in algorithm. I'll have to improve my understanding of the modules internals, because I'm certain we can find some ways to simplify this. |
Removing the LGTM until the bots build.
This looks to be taking a lot of effort to make work?
libcxx/include/algorithm | ||
---|---|---|
1746 | We could probably write a unit test to verify that the public headers include all of the private headers. We already have the list in private_headers in header_information.py. We'd just have to map the __fwd headers to their public header. |
Lol wait until you see the module map generator script in D144322. Maybe we did go overboard with nearly 1000 headers, but the private detail headers are necessary to prevent include and module cycles between the public headers.
I don't think we actually want all of these headers to be included in the top-level header. Looking at the changes, all of these are implementation-detail headers, which don't contain any public API, so there is no need for them to be included.
I think we do need them though. Previously importing e.g. std.type_traits would give you all of the detail headers. Once those are put in their own top level modules, they only get included if the public header includes them.
I don't see the problem. Users shouldn't ever refer to the stuff defined in these headers. If they do and splitting up the module breaks them, that's their problem, not ours.
Alright, let me try without this one. I still worry about the behavior change, but maybe it won't be noticeable since most of these submodules don't export anything. And it would let me sidestep the bizarre problem this causes in CI...
Not doing this adds loads of failures to D144322, but let me try to add more exports to fix them.
Do you have a link to this failed build? The latest failure is just a #include <__memory/uninitialized_buffer.h>, I assume you don't need help with that.
I'm still not sure why this issue doesn't show up in the modular builds we test in the CI.
It’s this one https://buildkite.com/llvm-project/libcxx-ci/builds/27284#018908b1-cc51-4a8b-af88-8bd36ded537b there’s a c++ module error that I can’t figure out
Thanks! The issue is that you added a new named declaration to <type_traits> and didn't export it from the std module. When they are no longer in sync this test fails.
However I'm not convinced the added named declaration is_execution_policy should be part of <type_traits>. Let's see what @philnik has to say.
libcxx/include/type_traits | ||
---|---|---|
473 | @philnik is_execution_policy is part of <execution>. I don't find the reason why this has been added to the __type_traits directory. Is that a bug? |
libcxx/include/type_traits | ||
---|---|---|
473 | That's a good question. I probably just put it there because it feels like a type trait, and I didn't think about where it should actually live. It should probably be moved to __execution/. |
I think you're misunderstanding what's happening. If it weren't for the header splitting we've been doing for the past 1-2 years, this whole effort would have been dead in the water since headers had cyclic dependencies. Breaking those cyclic dependencies between top-level headers is one of the reason for splitting the headers in the first place, and it would have been a pre-requisite for this work to happen.
Re this review, I think most of the includes are not desired. I would suggest turning this into a small "add missing includes" patch with only the few includes that were truly missing.
libcxx/include/algorithm | ||
---|---|---|
1738 | All of these seem to be unwanted. | |
libcxx/include/array | ||
121 | This one's good, the definition should include its forward declaration if there's one, I guess. | |
libcxx/include/chrono | ||
767–768 | No. | |
libcxx/include/format | ||
195 | I think all of those are no. | |
libcxx/include/functional | ||
533 | All those are internal utilities as well, so not needed! | |
libcxx/include/iterator | ||
678 | Same, not needed. | |
libcxx/include/memory | ||
876 | Same, not needed. | |
libcxx/include/memory_resource | ||
53 | Yes! | |
libcxx/include/ranges | ||
342 | All of those are internal, not needed. | |
libcxx/include/stop_token | ||
28 | Same, not needed. | |
libcxx/include/string | ||
554 | Same, not needed. | |
libcxx/include/thread | ||
85 | Same, not needed. | |
libcxx/include/tuple | ||
210 | Yes! | |
214 | I don't think we want this one. | |
214 | This one is no also, we use __tuple_like_ext here but not __tuple_like. | |
libcxx/include/type_traits | ||
429 | I think all of those are unwanted. | |
libcxx/include/utility | ||
244 | None of them seem needed. | |
libcxx/modules/std/type_traits.cppm | ||
228 | Can you explain why that's needed? This doesn't belong to <type_traits>, it belongs to <execution>. |
libcxx/include/memory_resource | ||
---|---|---|
53 | IMO __memory_resource/memory_resource.h should include the forward declaration, not the top-level header. |
libcxx/include/memory_resource | ||
---|---|---|
53 | Ah, you're right, I actually mis-read the name of the header this was added in. Yup, this one should go away then. |
libcxx/modules/std/type_traits.cppm | ||
---|---|---|
228 | It was needed if <type_traits> included <__type_traits/is_execution_policy.h>, but now that that's gone this can go too. |
All of these seem to be unwanted.