[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 marked an inline comment as done.
iana added inline comments.


================
Comment at: libcxx/include/cuchar:60
 
-using ::mbstate_t _LIBCPP_USING_IF_EXISTS;
 using ::size_t _LIBCPP_USING_IF_EXISTS;
----------------
Mordante wrote:
> Can you explain this change? Based on the synopsis above this must be provided in C++11 and later.
It redeclares `mbstate_t` from `<__mbstate_t.h>`. Modules can't deal with types being defined in multiple headers, so I used the canonical declaration in `<__mbstate_t.h>` instead of redeclaring  `mbstate_t` here.


================
Comment at: libcxx/include/module.modulemap.in:238-239
+      module min                             { export std___algorithm_min }
+      module min_element                     { export std___algorithm_min_element }
+      module min_max_result                  { export std_algorithm.__algorithm.min_max_result }
       module minmax                          {
----------------
Mordante wrote:
> iana wrote:
> > Mordante wrote:
> > > I see here two different ways used for the export. What is the difference between them and when use the first and when the second.
> > Some of the headers were ok to put in a top level `algorithm` module, but other ones had to be made their own top level module due to cycles with other modules. e.g. some __algorithm headers include __memory headers and vice versa. The libc++ headers are rather intertwined.
> > 
> > If we don't like the inconsistency, we could make all of the moved headers their own top level modules instead of trying to preserve the old structure like I did. It might be easier to maintain if they're all their own top level module too - it might be more difficult to introduce new module cycles.
> Thanks for updating the commit message. Can you also add more information which build configuration you use. We have a CI that tests Clang modules with C++2b, and this works fine. Looking at your changes I expect you use something different. So it would be good to know what you use.
> 
> My main concern of using the different methods is that it sounds error prone and I don't expect the libc++ CI to catch this. We might be able to solve that by adding a CI job.
> 
> If moving them all to a top-level reduces the risks of cycles, it sounds good. But before you do that overhaul I would like to hear @ldionne's opinion. Can you provide the benefits and drawbacks moving into that direction?
I'm using the Apple SDKs. It works in CI right now most likely because clang doesn't have modules for any of these headers yet. And it works in shipping Apple platforms because the Darwin module is marked as `[no_undeclared_includes]`, which prevents it from using the libc++ clang modules and seeing the cycles. That's a pretty bad thing, and technically the Apple SDKs don't work properly with clang modules and C++ because of it.

Once I add modules for the clang headers, the CI would most likely start seeing cycles without this change.

The libc++ CI is actually pretty good at catching cycles in the libc++ modules, that's the main way I was testing the change. Moving them all to top level reduces the risk of cycles, but it increases the risk of interdependent headers not working properly. e.g. `__iterator/iterator_traits.h` and `__iterator/readable_traits.h` each rely on the other and have to be in the same module or they won't work. So for that reason I opted with keeping things in their existing modules as much as possible.


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


================
Comment at: libcxx/test/support/test_macros.h:144
 #if defined(__cpp_lib_is_constant_evaluated) && __cpp_lib_is_constant_evaluated >= 201811L
+# include <__type_traits/is_constant_evaluated.h>
 # define TEST_IS_CONSTANT_EVALUATED std::is_constant_evaluated()
----------------
Mordante wrote:
> Mordante wrote:
> > Please remove this change. This only works for libc++, not for other Standard libraries. When this macro is used, the test should include `type_traits` itself.
> Sorry if I was not clear. But there should be no include here at all. This header is included by most tests, so we like to keep it lean, adding more dependencies is unwanted.
> 
> Instead when a test uses `TEST_IS_CONSTANT_EVALUATED` it should include `<type_traits>` itself.
> If this affects a lot of test I would prefer to have a separate review to fix these. Those changes are straight-forward and can be approved quickly.
It's not really a change though since <new> was previously including <type_traits> unconditionally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322