[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 inline comments.


================
Comment at: libcxx/include/cuchar:53
+#if !defined(_LIBCPP_CXX03_LANG)
+#  include <__mbstate_t.h>
+#endif
----------------
I think this helps readers of this header.


================
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:
> > 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.
I'm still a bit confused why our CI modular build[1]  doesn't catch these issues. Is there a way to have our current modular build catch these issues?

Is there a patch for adding modules for clang headers? I really would like to understand the problem better.

[1] https://buildkite.com/llvm-project/libcxx-ci/builds/19270#01866c01-7d50-4d07-a150-1ce852382dd8


================
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()
----------------
iana wrote:
> 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.
I don't understand what you mean. This header doesn't include `new`, or am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322