[libcxx-commits] [PATCH] D144322: [libc++][Modules] Make top level modules for all C++ headers with OS/clang versions

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_minmax.h:26-28
+#include <__type_traits/is_reference.h>
+#include <__type_traits/remove_cvref.h>
 #include <__utility/forward.h>
----------------
Can you please extract all the simple "missing includes" additions into its own patch? That one will sail through since we can just validate it by inspection, and it doesn't need any additional testing.


================
Comment at: libcxx/include/algorithm:1924-1931
 #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
-#  include <chrono>
+#  if !__has_feature(modules)
+// <chrono> creates an unbrekable module cycle from
+// <algorithm> -> <chrono> -> <format> -> <locale> ->
+// <mutex> -> <system_error> -> <string> -> <algorithm>
+#    include <chrono>
+#  endif
----------------
Instead, I would rather take the hit and unconditionally remove that transitive include in all standard modes. That's something we could do in LLVM 17, the impact is likely not a deal breaker and we have a good incentive to do it.


================
Comment at: libcxx/include/exception:386
+// <__memory/construct_at.h> -> <new> -> <exception> -> <type_traits>
+#    include <type_traits>
+#  endif
----------------
Same, I think we can probably kill this one unconditionally.


================
Comment at: libcxx/include/limits:831
+// <limits> -> <type_traits>
+#    include <type_traits>
+#  endif
----------------
Same. Direct users of `<limits>` are rare, I don't think this is going to bite too many people.


================
Comment at: libcxx/include/new:368
 #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
+#if !__has_feature(modules)
+// <type_traits> creates an unbrekable module cycle from
----------------
iana wrote:
> Mordante wrote:
> > I'm not fond of this. We try hard to make upgrading to newer libc++ versions smooth by not removing transitive includes when they are not needed. This change means that that no longer works for people using modules.
> > Would it be possible for you to add the compiler flag `-D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` in your build scripts?
> There isn't a very good alternative. Once the named headers are in different modules, their circular dependency becomes a module dependency which is a hard error for all modules clients. Scoping the change to only affect modules clients is the best we can do I think.
I think our best option here is to take the hit on those few headers that are causing major issues, like this one. That's something we should be able to roll out within the LLVM 17 dev period, and doing some large codebase rebuilds will also give us an idea of how bad this is in practice.

@iana Can you please extract all of these removals into a single review? You can name it something like "Remove unreconcilable circular dependencies in the headers", and we can land it on its own in preparation for the rest of the modules work. That will make it easier to review, test and roll out on its own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322