[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/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:
> > > > 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
> The CI doesn't catch the issues yet because the clang headers don't have modules, and Apple's Darwin module has `[no_undeclared_includes]`. So right now one of two things will happen.
> 
>   # The host OS doesn't have module maps for usr/include headers. This means that when the `std` module is built, clang's e.g. stdint.h doesn't have a module, and the OS's stdint.h also doesn't have a module. When the `std` module is built and libc++'s stdint.h includes the nonmodular clang and OS versions, they will be absorbed into the `std` module.
>   # The host OS does have a module map for usr/include headers, i.e. Apple's Darwin module. Most likely these modules are `[no_undeclared_includes]` like the Darwin module, otherwise they would be seeing errors. The clang headers are somewhat modular in this scenario. `ModuleMap::isBuiltinHeader` will allow a set list of clang headers to be absorbed into the OS modules, and the rest are absorbed into the `std` module. `[no_undeclared_includes]` prevents cycles by disallowing the Darwin module from importing any other module, i.e. `std`. The problem with `[no_undeclared_includes]` is that it breaks some C++ requirements. e.g. including stdlib.h from a modular C++ client on Apple platforms won't give you the C++ version of stddef.h, because importing the `std` module (libc++'s stddef.h) is blocked from the Darwin module (Apple's stdlib.h includes stddef.h). You silently get the OS version of stddef.h instead, and things are subtly weird/don't work quite right for C++ clients.
> 
> I'm still working on the patch for adding modules to the clang headers. Those would break the CI without this change, because it will cause the clang headers to become modular and //not// be absorbed into the `std` module, and so the cycles would appear.
Thanks! This makes things clearer for me.


================
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:
> > 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?
> Sorry I was thinking of a different header, never mind my last comment. I'm actually not sure how this ever worked. Why do we want to make all of the clients responsible for including the dependencies of test_macros.h though?
Since most clients don't use this macro. This header contains mainly configuration information regarding available features depending on the compiler, C++ version, and OS. This is included by most tests, so we prefer to keep it small to reduce the parse overhead in the test. As said before, when this breaks things for you best make a separate patch and we can approve that quickly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322