[NFC][libc++] Moves transitive includes location.

[Mordante]

Summary

This moves some includes missed in D133212.

[philnik]

Why would we want to do this? We don't include the normal headers outside the include guard, and AFAICT the only thing this changes is that the compiler potentially has to load the files more often just to do nothing. That shouldn't be a problem, since the compiler optimizes that AFAIK, but why even open the possibility?

[Mordante]

In D135180#3834249, @philnik wrote:

Why would we want to do this? We don't include the normal headers outside the include guard, and AFAICT the only thing this changes is that the compiler potentially has to load the files more often just to do nothing. That shouldn't be a problem, since the compiler optimizes that AFAIK, but why even open the possibility?

I have a slight preference for the current location. But @ldionne preferred to have them completely at the end.
The compiler indeed has ways include optimizations, which we discovered while working on the transitive includes.

[ldionne]

In D135180#3834327, @Mordante wrote:

In D135180#3834249, @philnik wrote:

Why would we want to do this? We don't include the normal headers outside the include guard, and AFAICT the only thing this changes is that the compiler potentially has to load the files more often just to do nothing. That shouldn't be a problem, since the compiler optimizes that AFAIK, but why even open the possibility?

I have a slight preference for the current location. But @ldionne preferred to have them completely at the end.
The compiler indeed has ways include optimizations, which we discovered while working on the transitive includes.

Reasons for doing it this way:

1. Less chance of depending on transitive includes without realizing it
2. This can solve circular dependency issues. For example, if <vector> uses std::copy via <algorithm> but <algorithm> also uses std::vector via <vector> (to implement something unrelated to std::copy), including <algorithm> at the top of <vector> will cause an error, because you'll parse the top of <vector>, then all of <algorithm>, and you'll fail to find a declaration of std::vector. If you do it this way, you'll parse all of <vector>, and then at the end you'll include <algorithm>, which will try to include <vector> again without success, but you'll have already defined std::vector so it's fine.

[philnik]

In D135180#3835058, @ldionne wrote:

In D135180#3834327, @Mordante wrote:

In D135180#3834249, @philnik wrote:

Why would we want to do this? We don't include the normal headers outside the include guard, and AFAICT the only thing this changes is that the compiler potentially has to load the files more often just to do nothing. That shouldn't be a problem, since the compiler optimizes that AFAIK, but why even open the possibility?

I have a slight preference for the current location. But @ldionne preferred to have them completely at the end.
The compiler indeed has ways include optimizations, which we discovered while working on the transitive includes.

Reasons for doing it this way:

1. Less chance of depending on transitive includes without realizing it
2. This can solve circular dependency issues. For example, if <vector> uses std::copy via <algorithm> but <algorithm> also uses std::vector via <vector> (to implement something unrelated to std::copy), including <algorithm> at the top of <vector> will cause an error, because you'll parse the top of <vector>, then all of <algorithm>, and you'll fail to find a declaration of std::vector. If you do it this way, you'll parse all of <vector>, and then at the end you'll include <algorithm>, which will try to include <vector> again without success, but you'll have already defined std::vector so it's fine.

I think you misunderstood what I'm questioning. I'm not complaining about moving the includes to the bottom of the header - that's a good idea; I'm complaining about moving them outside the include guard. I don't see and benefit in doing that, but it might cause the compiler to load the files more often than necessary, since the include guard doesn't cover them anymore.

[ldionne]

In D135180#3835200, @philnik wrote:

I think you misunderstood what I'm questioning. I'm not complaining about moving the includes to the bottom of the header - that's a good idea; I'm complaining about moving them outside the include guard. I don't see and benefit in doing that, but it might cause the compiler to load the files more often than necessary, since the include guard doesn't cover them anymore.

Oh. I must say I don't remember why and don't understand why I left the original comment that triggered this change: https://reviews.llvm.org/D133212#inline-1285216. I must have thought that keeping the includes inside the header guards would mean that the cyclic includes problem isn't fixed, which isn't true.

Regardless, I agree that this would probably inhibit the header guard optimization from triggering, which would be rather bad. I think we should keep the parts of this change that move transitive includes from the top to the bottom of the header, but drop the parts that move them outside of the include guards. Sorry for my misleading comment on D133212.

[ldionne]

In D135180#3849970, @ldionne wrote:

Regardless, I agree that this would probably inhibit the header guard optimization from triggering, which would be rather bad. I think we should keep the parts of this change that move transitive includes from the top to the bottom of the header, but drop the parts that move them outside of the include guards. Sorry for my misleading comment on D133212.

Also, that wasn't necessarily clear from the previous interactions on this patch, but I originally thought this was just moving stuff from the top to the bottom of includes -- it didn't click that the patch was actually moving stuff from inside to outside the header guards.

[Mordante]

Rebased and addresses review comments.

[Mordante]

In D135180#3835200, @philnik wrote:

In D135180#3835058, @ldionne wrote:

In D135180#3834327, @Mordante wrote:

In D135180#3834249, @philnik wrote:

Why would we want to do this? We don't include the normal headers outside the include guard, and AFAICT the only thing this changes is that the compiler potentially has to load the files more often just to do nothing. That shouldn't be a problem, since the compiler optimizes that AFAIK, but why even open the possibility?

I have a slight preference for the current location. But @ldionne preferred to have them completely at the end.
The compiler indeed has ways include optimizations, which we discovered while working on the transitive includes.

Reasons for doing it this way:

1. Less chance of depending on transitive includes without realizing it
2. This can solve circular dependency issues. For example, if <vector> uses std::copy via <algorithm> but <algorithm> also uses std::vector via <vector> (to implement something unrelated to std::copy), including <algorithm> at the top of <vector> will cause an error, because you'll parse the top of <vector>, then all of <algorithm>, and you'll fail to find a declaration of std::vector. If you do it this way, you'll parse all of <vector>, and then at the end you'll include <algorithm>, which will try to include <vector> again without success, but you'll have already defined std::vector so it's fine.

I think you misunderstood what I'm questioning. I'm not complaining about moving the includes to the bottom of the header - that's a good idea; I'm complaining about moving them outside the include guard. I don't see and benefit in doing that, but it might cause the compiler to load the files more often than necessary, since the include guard doesn't cover them anymore.

Thanks for catching this @philnik. I've discussed this with @ldionne yesterday and I only left the moving of the transitive included to the end of the file inside the include guards.

[philnik]

LGTM with green CI.

libcxx/include/barrier

 #include <__availability>
 #include <__config>
 #include <__memory/unique_ptr.h>
 #include <__thread/timed_backoff_policy.h>
 #include <__utility/move.h>
 #include <atomic>
 #include <limits>
 
-#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
-#  include <concepts>
-#  include <iterator>
-#  include <memory>
-#  include <stdexcept>
-#  include <variant>
-#endif
-
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #ifdef _LIBCPP_HAS_NO_THREADS
 # error "<barrier> is not supported since libc++ has been configured without support for threads."
 #endif
 

 };
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP_STD_VER >= 14
 
 _LIBCPP_POP_MACROS
 
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
+#  include <concepts>
+#  include <iterator>
+#  include <memory>
+#  include <stdexcept>
+#  include <variant>
+#endif
+
 #endif //_LIBCPP_BARRIER

libcxx/include/experimental/memory_resource

 #include <experimental/__config>
 #include <experimental/__memory>
 #include <limits>
 #include <new>
 #include <stdexcept>
 #include <tuple>
 #include <type_traits>
 
-#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
-#  include <atomic>
-#  include <climits>
-#  include <concepts>
-#  include <cstring>
-#  include <ctime>
-#  include <iterator>
-#  include <memory>
-#  include <ratio>
-#  include <variant>
-#endif
-
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_LFTS_PMR
using resource_adaptor = __resource_adaptor_imp<
   >;
 
 #endif // _LIBCPP_CXX03_LANG
 
 _LIBCPP_END_NAMESPACE_LFTS_PMR
 
 _LIBCPP_POP_MACROS
 
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
+#  include <atomic>
+#  include <climits>
+#  include <concepts>
+#  include <cstring>
+#  include <ctime>
+#  include <iterator>
+#  include <memory>
+#  include <ratio>
+#  include <variant>
+#endif
+
 #endif /* _LIBCPP_EXPERIMENTAL_MEMORY_RESOURCE */

libcxx/include/scoped_allocator

 #include <__type_traits/remove_reference.h>
 #include <__utility/forward.h>
 #include <__utility/move.h>
 #include <__utility/pair.h>
 #include <__utility/piecewise_construct.h>
 #include <tuple>
 #include <version>
 
-#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
-#  include <atomic>
-#  include <climits>
-#  include <concepts>
-#  include <cstring>
-#  include <ctime>
-#  include <iterator>
-#  include <memory>
-#  include <ratio>
-#  include <stdexcept>
-#  include <variant>
-#endif
-
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_CXX03_LANG)
 

 {
     return !(__a == __b);
 }
 
 #endif // !defined(_LIBCPP_CXX03_LANG)
 
 _LIBCPP_END_NAMESPACE_STD
 
+#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
+#  include <atomic>
+#  include <climits>
+#  include <concepts>
+#  include <cstring>
+#  include <ctime>
+#  include <iterator>
+#  include <memory>
+#  include <ratio>
+#  include <stdexcept>
+#  include <variant>
+#endif
+
 #endif // _LIBCPP_SCOPED_ALLOCATOR