This is an archive of the discontinued LLVM Phabricator instance.

[libc++][Modules] Add missing __fwd includes
ClosedPublic

Authored by iana on Jun 17 2023, 6:57 PM.

Details

Summary

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.

Diff Detail

Event Timeline

iana created this revision.Jun 17 2023, 6:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2023, 6:57 PM
Herald added a subscriber: ributzka. · View Herald Transcript
iana requested review of this revision.Jun 17 2023, 6:57 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
iana updated this revision to Diff 532515.Jun 18 2023, 7:57 PM

Reordering in the stack

iana updated this revision to Diff 534250.Jun 24 2023, 11:32 AM

Update transitive includes

iana updated this revision to Diff 534652.Jun 26 2023, 10:58 AM

Don't include the textual headers in algorithm

iana updated this revision to Diff 535056.Jun 27 2023, 10:54 AM

Update the headers

iana updated this revision to Diff 535238.Jun 27 2023, 11:03 PM

Remove a redundant include

iana updated this revision to Diff 535245.Jun 27 2023, 11:39 PM

Restore an accidentally deleted comment

iana added a comment.Jun 29 2023, 3:47 PM

@Mordante @ldionne do either of you understand the CI failure? I can't make heads or tails of it...

EricWF accepted this revision.Jun 29 2023, 6:51 PM
EricWF added a subscriber: EricWF.

@ldionne @philnik

The complexity of having hundreds of headers is starting to show its head.
How do we prevent this class of bugs from continuing to occur?

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.
This furthers my concern that we've invented a bunch of complexity which only debatably pulls its weight.

I'll have to improve my understanding of the modules internals, because I'm certain we can find some ways to simplify this.

This revision is now accepted and ready to land.Jun 29 2023, 6:51 PM
EricWF requested changes to this revision.Jun 29 2023, 6:53 PM

Removing the LGTM until the bots build.
This looks to be taking a lot of effort to make work?

This revision now requires changes to proceed.Jun 29 2023, 6:53 PM
iana added a comment.Jun 29 2023, 9:35 PM

Removing the LGTM until the bots build.
This looks to be taking a lot of effort to make work?

Yes it's really taken quite a lot of effort to get it to work.

iana added inline comments.Jun 29 2023, 10:30 PM
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.

iana added a comment.Jun 29 2023, 10:37 PM

The complexity of having hundreds of headers is starting to show its head.

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.

philnik requested changes to this revision.Jun 30 2023, 9:38 AM

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.

iana added a comment.EditedJun 30 2023, 1:59 PM

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 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.

iana added a comment.Jun 30 2023, 3:15 PM

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...

iana added a comment.Jun 30 2023, 3:40 PM

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.

@Mordante @ldionne do either of you understand the CI failure? I can't make heads or tails of it...

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.

@ldionne @philnik

The complexity of having hundreds of headers is starting to show its head.
How do we prevent this class of bugs from continuing to occur?

I'm still not sure why this issue doesn't show up in the modular builds we test in the CI.

iana added a comment.Jul 1 2023, 8:02 AM

@Mordante @ldionne do either of you understand the CI failure? I can't make heads or tails of it...

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.

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

@Mordante @ldionne do either of you understand the CI failure? I can't make heads or tails of it...

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.

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?

philnik added inline comments.Jul 1 2023, 10:39 AM
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/.

iana updated this revision to Diff 536830.Jul 3 2023, 10:19 AM

Add the missing using to the type_traits module

iana updated this revision to Diff 536898.Jul 3 2023, 2:24 PM

Remove a removed header

iana updated this revision to Diff 536954.Jul 3 2023, 10:50 PM

Fix a typo

@ldionne @philnik

The complexity of having hundreds of headers is starting to show its head.
How do we prevent this class of bugs from continuing to occur?

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>.

philnik added inline comments.Jul 5 2023, 12:05 PM
libcxx/include/memory_resource
53

IMO __memory_resource/memory_resource.h should include the forward declaration, not the top-level header.

ldionne added inline comments.Jul 5 2023, 2:00 PM
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.

iana marked 22 inline comments as done.Jul 5 2023, 2:58 PM
iana added inline comments.
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.

iana updated this revision to Diff 537517.Jul 5 2023, 2:59 PM
iana marked an inline comment as done.

Update for review feedback

iana retitled this revision from [libc++][Modules] Add missing includes to public headers to [libc++][Modules] Add missing __fwd includes.Jul 5 2023, 2:59 PM
iana edited the summary of this revision. (Show Details)
philnik accepted this revision.Jul 5 2023, 3:04 PM

Thanks for the cleanup. LGTM. Let's wait a day or two before landing in case @Mordante or @ldionne have any more comments.

iana updated this revision to Diff 537855.Jul 6 2023, 1:23 PM

Rebase

ldionne accepted this revision.Jul 7 2023, 11:25 AM
ldionne accepted this revision.Jul 7 2023, 1:35 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 7 2023, 2:09 PM
This revision was automatically updated to reflect the committed changes.