[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 a comment.

In D144322#4136898 <https://reviews.llvm.org/D144322#4136898>, @Mordante wrote:

> I have not reviewed the entire patch.
> Please update the commit message to describe why these changes are needed and what the benefit is. It looks some parts are wrong; removing headers for forward declarations, while the contents of the header are used, is wrong. It might compile due to transitive includes, but we really should include the proper headers.

Updated the commit message, I'll circle back on the forward declarations in the __format headers.



================
Comment at: libcxx/include/__format/format_arg.h:24-25
 #include <__variant/monostate.h>
-#include <string>
-#include <string_view>
 
----------------
Mordante wrote:
> Why are these two replaced with the `__fwd` version? The `string_view` seems wrong, since it's used as a member variable.
These were to break up a module cycle from string -> string_view -> ... -> chrono -> format -> string. But let me see if I can find another way to break it.


================
Comment at: libcxx/include/module.modulemap.in:1
-// define the module for __config outside of the top level 'std' module
-// since __config may be included from C headers which may create an
----------------
Mordante wrote:
> Changing this file will cause merge conflicts with several in-flight patches. That is not a blocker per se, however I really want the commit message explain what the benefit of this change is.
Oh I know, I've had to fix several conflicts while working on this change.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322