[libcxx-commits] [PATCH] D153211: [libc++][Modules] Make module exports consistent with header includes

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/iterator_operations.h:23
 #include <__iterator/prev.h>
-#include <__iterator/readable_traits.h>
 #include <__type_traits/enable_if.h>
----------------
Let's move the definition of `iter_value_t` from `libcxx/include/__iterator/readable_traits.h` to `libcxx/include/__iterator/iterator_traits.h` and fix up the existing includes of `<__iterator/readable_traits.h>`. This will remove the dependency of `readable_traits.h` on `iterator_traits.h`. This can be done in a separate patch.


================
Comment at: libcxx/include/__algorithm/ranges_partial_sort.h:12
 
+#include <__algorithm/in_out_result.h>
 #include <__algorithm/iterator_operations.h>
----------------
I think we want to *not* include this header and instead remove `export algorithm.__algorithm.in_out_result` from `ranges_partial_sort` (but not for `ranges_partial_sort_copy` and others).

Basically I think adding that export was a mistake that was made in https://reviews.llvm.org/D136711.


================
Comment at: libcxx/include/__algorithm/ranges_partial_sort_copy.h:12
 
+#include <__algorithm/in_out_out_result.h>
 #include <__algorithm/in_out_result.h>
----------------
I think this one shouldn't be added, but instead we should change `export algorithm.__algorithm.in_out_out_result` to `export algorithm.__algorithm.in_out_result` in `module ranges_partial_sort_copy`.


================
Comment at: libcxx/include/__chrono/high_resolution_clock.h:15
 #include <__chrono/system_clock.h>
+#include <__chrono/time_point.h>
 #include <__config>
----------------
I think we don't want to add this one, but instead we want to

```
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index d49e339f2fbb..bac76a70ff4f 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -763,7 +763,6 @@ module std [system] {
         private header "__chrono/high_resolution_clock.h"
         export steady_clock
         export system_clock
-        export time_point
       }
       module literals               { private header "__chrono/literals.h" }
       module month                  { private header "__chrono/month.h" }
@@ -778,8 +777,14 @@ module std [system] {
         private header "__chrono/parser_std_format_spec.h"
       }
       module statically_widen       { private header "__chrono/statically_widen.h" }
-      module steady_clock           { private header "__chrono/steady_clock.h" }
-      module system_clock           { private header "__chrono/system_clock.h" }
+      module steady_clock           {
+        private header "__chrono/steady_clock.h"
+        export time_point
+      }
+      module system_clock           {
+        private header "__chrono/system_clock.h"
+        export time_point
+      }
       module time_point             { private header "__chrono/time_point.h" }
       module weekday                { private header "__chrono/weekday.h" }
       module year                   { private header "__chrono/year.h" }
```


================
Comment at: libcxx/include/__filesystem/path.h:17
 #include <__config>
+#include <__functional/hash.h>
 #include <__functional/unary_function.h>
----------------
This one is definitely correct, it was a missing include.


================
Comment at: libcxx/include/__format/format_context.h:24
 #include <__iterator/concepts.h>
+#include <__locale>
 #include <__memory/addressof.h>
----------------
iana wrote:
> Mordante wrote:
> > I doubt this is needed, this is part of `<locale>` and should be exported by `<locale>`. When this is really needed it should be moved to line 31 in the `_LIBCPP_HAS_NO_LOCALIZATION`. Otherwise the CI will fail.
> I added this because `std.format.__format.format_context` exports `__locale` (aka `std.__locale`). This currently works because they're in the same top level module, but when `__format/format_context.h` and `__locale` belong to different top level modules, the export can have no effect. (Sometimes it works and sometimes it doesn't. I think if the export is transitively included by one of the module's headers then it does work, but that's fragile.) Most of the includes added in this review are due to similar circumstances: the module for the header was exporting the module for the include. Maybe not all of the exports need to be there, but it's too difficult to track down which ones are necessary after the mega module breakup.
Instead, I think `locale` should export `__locale` and `format_context` should only `export locale`, not `export __locale`.


================
Comment at: libcxx/include/__ranges/all.h:14-15
 #include <__config>
+#include <__functional/compose.h>
+#include <__functional/perfect_forward.h>
 #include <__iterator/concepts.h>
----------------
Those don't make sense to me but I can't figure out what's wrong. The solution *should* be to remove the exports from `module all` but it doesn't work for some reason.


================
Comment at: libcxx/include/__system_error/error_condition.h:15
 #include <__config>
+#include <__functional/hash.h>
 #include <__functional/unary_function.h>
----------------
This is adding a missing include -- obviously correct.


================
Comment at: libcxx/utils/libcxx/test/header_information.py:149-152
+                                                                     # Textual headers shouldn't be included directly
+                                                                     # (they're typically part of another header that
+                                                                     # was split out just for organization).
+                                                                     and str(p.relative_to(include)) not in private_textual_headers
----------------
Is this still going to be required after we make `__iterator/readable_traits.h` a standalone header?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153211