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

[iana]

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.

[iana]

Reordering in the stack

[iana]

Rebase

[iana]

Rebase

[iana]

Rebase

[iana]

Rebase

[iana]

Rebase

[iana]

Rebase

[iana]

Rebase

[iana]

Rebase

[iana]

Update transitive includes

[iana]

Don't include the textual headers in algorithm

[iana]

Rebase

[iana]

Update the headers

[iana]

Rebase

[iana]

Remove a redundant include

[iana]

Restore an accidentally deleted comment

[iana]

Rebase

[iana]

Rebase

[iana]

Rebase

[iana]

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

[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

1747 ↗

(On Diff #535908)

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.

[EricWF]

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

[iana]

In D153216#4462380, @EricWF wrote:

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]

libcxx/include/algorithm

1747 ↗

(On Diff #535908)

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]

In D153216#4462376, @EricWF wrote:

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]

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]

Rebase

[iana]

In D153216#4464361, @philnik wrote:

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.

[philnik]

In D153216#4465367, @iana wrote:

In D153216#4464361, @philnik wrote:

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]

In D153216#4465444, @philnik wrote:

In D153216#4465367, @iana wrote:

In D153216#4464361, @philnik wrote:

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]

In D153216#4465685, @iana wrote:

In D153216#4465444, @philnik wrote:

In D153216#4465367, @iana wrote:

In D153216#4464361, @philnik wrote:

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]

In D153216#4461976, @iana wrote:

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

In D153216#4462376, @EricWF wrote:

@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]

In D153216#4466234, @Mordante wrote:

In D153216#4461976, @iana wrote:

@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]

In D153216#4466307, @iana wrote:

In D153216#4466234, @Mordante wrote:

In D153216#4461976, @iana wrote:

@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 ↗

(On Diff #536370)

@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]

libcxx/include/type_traits

473 ↗

(On Diff #536370)

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]

Add the missing using to the type_traits module

[iana]

Remove a removed header

[iana]

Fix a typo

[iana]

Rebase

[ldionne]

In D153216#4462376, @EricWF wrote:

@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 ↗

(On Diff #537219)

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 ↗

(On Diff #537219)

No.

libcxx/include/format

195 ↗

(On Diff #537219)

I think all of those are no.

libcxx/include/functional

533 ↗

(On Diff #537219)

All those are internal utilities as well, so not needed!

libcxx/include/iterator

678 ↗

(On Diff #537219)

Same, not needed.

libcxx/include/memory

876 ↗

(On Diff #537219)

Same, not needed.

libcxx/include/memory_resource

53 ↗

(On Diff #537219)

Yes!

libcxx/include/ranges

342 ↗

(On Diff #537219)

All of those are internal, not needed.

libcxx/include/stop_token

28 ↗

(On Diff #537219)

Same, not needed.

libcxx/include/string

554 ↗

(On Diff #537219)

Same, not needed.

libcxx/include/thread

85 ↗

(On Diff #537219)

Same, not needed.

libcxx/include/tuple

210

Yes!

215

I don't think we want this one.

218

This one is no also, we use __tuple_like_ext here but not __tuple_like.

libcxx/include/type_traits

429 ↗

(On Diff #537219)

I think all of those are unwanted.

libcxx/include/utility

244 ↗

(On Diff #537219)

None of them seem needed.

libcxx/modules/std/type_traits.cppm

226 ↗

(On Diff #537219)

Can you explain why that's needed? This doesn't belong to <type_traits>, it belongs to <execution>.

[philnik]

libcxx/include/memory_resource

53 ↗

(On Diff #537219)

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

[ldionne]

libcxx/include/memory_resource

53 ↗

(On Diff #537219)

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]

libcxx/modules/std/type_traits.cppm

226 ↗

(On Diff #537219)

It was needed if <type_traits> included <__type_traits/is_execution_policy.h>, but now that that's gone this can go too.

[iana]

Update for review feedback

[philnik]

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

[iana]

Rebase

libcxx/include/__memory_resource/memory_resource.h

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___MEMORY_RESOURCE_MEMORY_RESOURCE_H
 #define _LIBCPP___MEMORY_RESOURCE_MEMORY_RESOURCE_H
 
 #include <__availability>
 #include <__config>
+#include <__fwd/memory_resource.h>
 #include <cstddef>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #if _LIBCPP_STD_VER >= 17
 

libcxx/include/array

 
 #include <__algorithm/equal.h>
 #include <__algorithm/fill_n.h>
 #include <__algorithm/lexicographical_compare.h>
 #include <__algorithm/lexicographical_compare_three_way.h>
 #include <__algorithm/swap_ranges.h>
 #include <__assert> // all public C++ headers provide the assertion handler
 #include <__config>
+#include <__fwd/array.h>
 #include <__iterator/reverse_iterator.h>
 #include <__tuple/sfinae_helpers.h>
 #include <__type_traits/conditional.h>
 #include <__type_traits/is_array.h>
 #include <__type_traits/is_const.h>
 #include <__type_traits/is_constructible.h>
 #include <__type_traits/is_move_constructible.h>
 #include <__type_traits/is_nothrow_constructible.h>

libcxx/include/tuple

 */
 
 #include <__assert> // all public C++ headers provide the assertion handler
 #include <__compare/common_comparison_category.h>
 #include <__compare/synth_three_way.h>
 #include <__config>
 #include <__functional/invoke.h>
 #include <__fwd/array.h>
+#include <__fwd/get.h>
 #include <__fwd/tuple.h>
 #include <__memory/allocator_arg_t.h>
 #include <__memory/uses_allocator.h>
 #include <__tuple/make_tuple_types.h>
 #include <__tuple/sfinae_helpers.h>
 #include <__tuple/tuple_element.h>
 #include <__tuple/tuple_indices.h>
 #include <__tuple/tuple_like_ext.h>
 #include <__tuple/tuple_size.h>
 #include <__tuple/tuple_types.h>
 #include <__type_traits/apply_cv.h>
 #include <__type_traits/common_reference.h>
 #include <__type_traits/common_type.h>
 #include <__type_traits/conditional.h>
 #include <__type_traits/conjunction.h>
 #include <__type_traits/copy_cvref.h>