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

[Ian Anderson via Phabricator via libcxx-commits]

iana planned changes to this revision.
iana marked 8 inline comments as done.
iana added a subscriber: var-const.
iana added a comment.
Herald added a subscriber: jplehr.

Discussed this some more with @ldionne, @Bigcheese, @vsapsai, @var-const. We don't love this patch for a few reasons.

1. It looks arbitrary what got left in `std` and what got pulled out to top level.
2. It looks arbitrary which private detail headers are in the new `std_abc` modules and which ones are top level modules.
3. Figuring out where the private detail headers go manually is tricky, and possibly difficult to maintain. (I've rebased this a couple of times over the last few months and it keeps adding module cycles that have been tough to resolve. It might be less difficult if the cycles get resolved as the headers are changed, or it might just always be hard.)

The two ideas we had to improve the situation are these.

1. Make a top level module for all of the headers. This is the simple approach and, if there aren't any include cycles, will be the easiest way to avoid module cycles. But it makes ~950 top level modules which looks kind of goofy and has a fair chance at sub-optimal performance.
2. Make top level modules for all the public headers, and programmatically generate the minimum number of top level modules for the private detail headers at build time such that there are no cycles. I'm still working on this one.

We also don't think that it's important to keep the current `std` module, we don't think there are any Objective-C++ clients that are doing `@import std;`.



================
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>
----------------
ldionne wrote:
> 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.
Yep I'll do that.


================
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
----------------
Mordante wrote:
> ldionne wrote:
> > 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.
> I would prefer to do that in a separate patch and notify the libc++ vendors and announce it on Discourse. (And obviously it should be in the release notes.) The same for the other transitive includes removals.
> 
> Edit I see @ldionne wrote something similar in a different comment.
Sounds good, I'll make a separate review for this one too. I think some of the detail headers have been changed and this might be the only remaining cycle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322