[libcxx-commits] [PATCH] D153216: [libc++][Modules] Add missing includes to public headers

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D153216#4462376 <https://reviews.llvm.org/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.



================
Comment at: libcxx/include/algorithm:1738
 */
 
 #include <__assert> // all public C++ headers provide the assertion handler
----------------
All of these seem to be unwanted.


================
Comment at: libcxx/include/array:121
 #include <__config>
+#include <__fwd/array.h>
 #include <__iterator/reverse_iterator.h>
----------------
This one's good, the definition should include its forward declaration if there's one, I guess.


================
Comment at: libcxx/include/chrono:767
 #include <__assert> // all public C++ headers provide the assertion handler
 #include <__chrono/calendar.h>
+#include <__chrono/concepts.h>
----------------
No.


================
Comment at: libcxx/include/format:195
 #include <__format/formatter_integer.h>
+#include <__format/formatter_integral.h>
+#include <__format/formatter_output.h>
----------------
I think all of those are no.


================
Comment at: libcxx/include/functional:533
 #include <__functional/invoke.h>
+#include <__functional/is_transparent.h>
 #include <__functional/mem_fn.h> // TODO: deprecate
----------------
All those are internal utilities as well, so not needed!


================
Comment at: libcxx/include/iterator:678
 #include <__assert> // all public C++ headers provide the assertion handler
 #include <__config>
 #include <__iterator/access.h>
----------------
Same, not needed.


================
Comment at: libcxx/include/memory:876
 
 }  // std
 
----------------
Same, not needed.


================
Comment at: libcxx/include/memory_resource:53
 #include <__config>
+#include <__fwd/memory_resource.h>
 #include <__memory_resource/memory_resource.h>
----------------
Yes!


================
Comment at: libcxx/include/ranges:342
   };
 
   struct from_range_t { explicit from_range_t() = default; };  // Since C++23
----------------
All of those are internal, not needed.


================
Comment at: libcxx/include/stop_token:28
 
   // [stopcallback], class template stop_callback
   template<class Callback>
----------------
Same, not needed.


================
Comment at: libcxx/include/string:554
 #include <__memory/allocate_at_least.h>
 #include <__memory/allocator.h>
 #include <__memory/allocator_traits.h>
----------------
Same, not needed.


================
Comment at: libcxx/include/thread:85
 
 }  // std
 
----------------
Same, not needed.


================
Comment at: libcxx/include/tuple:210
 #include <__fwd/array.h>
+#include <__fwd/get.h>
 #include <__fwd/tuple.h>
----------------
Yes!


================
Comment at: libcxx/include/tuple:215
 #include <__tuple/make_tuple_types.h>
+#include <__tuple/pair_like.h>
 #include <__tuple/sfinae_helpers.h>
----------------
I don't think we want this one.


================
Comment at: libcxx/include/tuple:219
 #include <__tuple/tuple_indices.h>
+#include <__tuple/tuple_like.h>
 #include <__tuple/tuple_like_ext.h>
----------------
This one is no also, we use `__tuple_like_ext` here but not `__tuple_like`.


================
Comment at: libcxx/include/type_traits:429
 #include <__type_traits/aligned_storage.h>
 #include <__type_traits/aligned_union.h>
 #include <__type_traits/alignment_of.h>
----------------
I think all of those are unwanted.


================
Comment at: libcxx/include/utility:244
     constexpr underlying_type_t<T> to_underlying( T value ) noexcept; // C++23
 
 }  // std
----------------
None of them seem needed.


================
Comment at: libcxx/modules/std/type_traits.cppm:226
   using std::is_compound_v;
+  using std::is_execution_policy_v;
   using std::is_fundamental_v;
----------------
Can you explain why that's needed? This doesn't belong to `<type_traits>`, it belongs to `<execution>`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153216/new/

https://reviews.llvm.org/D153216