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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I went over the patch, there are some minor issues. I really like to see what our CI thinks of the changes.

I really like to have a bit more information on the changes to the module map itself.



================
Comment at: libcxx/include/algorithm:1925-1931
+#if !__has_feature(modules)
+// <chrono> creates an unbrekable module cycle from
+// <algorithm> -> <chrono> -> <format> -> <locale> ->
+// <mutex> -> <system_error> -> <string> -> <algorithm>
 #  include <chrono>
 #endif
+#endif
----------------
Please run clang-format on this hunk. Not all our code is formatted, but this file is. This change is what breaks the generated output step.


================
Comment at: libcxx/include/cuchar:60
 
-using ::mbstate_t _LIBCPP_USING_IF_EXISTS;
 using ::size_t _LIBCPP_USING_IF_EXISTS;
----------------
Can you explain this change? Based on the synopsis above this must be provided in C++11 and later.


================
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                          {
----------------
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?


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322