This is an archive of the discontinued LLVM Phabricator instance.

[libc++][Modules] Make module exports consistent with header includes
ClosedPublic

Authored by iana on Jun 17 2023, 6:55 PM.

Details

Summary

Some modules export modules that they don't import (i.e. that their header doesn't directly include). That sometimes works when the exported submodule is in the same module, but when the std mega module is broken up (D144322), some of the exports stop working. Make the exports and includes consistent, either by adding includes for the exports, or by removing exports for missing includes.

The concepts.equality_comparable export in std.iterator.__iterator.concepts isn't doing anything because 1) it's resolved as std.iterator.__iterator.concepts.equality_comparable and 2) there's a __concepts submodule in between std.concepts and equality_comparable. Fix it to be std.concepts.__concepts.equality_comparable.

<span> is listed in both std.span and std.experimental.span. Delete the latter module.
There is no __errc module or header, so remove that export from std.system_error.

Diff Detail

Event Timeline

iana created this revision.Jun 17 2023, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2023, 6:55 PM
iana requested review of this revision.Jun 17 2023, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2023, 6:55 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added inline comments.Jun 18 2023, 7:48 AM
libcxx/include/__format/format_context.h
24 ↗(On Diff #532438)

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.

libcxx/include/cwchar
113

Is this really needed, it looks weird; C headers should be independent.

libcxx/include/mdspan
64–65 ↗(On Diff #532438)

Can you move these to the __mdspan/extents.h where the are really needed?

libcxx/include/string_view
230

The header does not use initializer_list why is it needed?

libcxx/utils/libcxx/test/header_information.py
76 ↗(On Diff #532438)

I really like to see comment why this file is in this list.

iana updated this revision to Diff 532504.Jun 18 2023, 7:31 PM

Reordered in the stack since the exports are now needed to revert some of the added includes that don't work.

iana added inline comments.Jun 18 2023, 7:55 PM
libcxx/include/__format/format_context.h
24 ↗(On Diff #532438)

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.

iana updated this revision to Diff 532720.Jun 19 2023, 11:31 AM

Fix unit tests

iana updated this revision to Diff 533021.Jun 20 2023, 12:48 PM

Fix unit tests, reorder in the stack

iana updated this revision to Diff 533075.Jun 20 2023, 4:06 PM

Re-order the stack again

iana updated this revision to Diff 533107.Jun 20 2023, 7:38 PM

More CI fixes

iana edited the summary of this revision. (Show Details)Jun 21 2023, 12:44 PM
iana edited the summary of this revision. (Show Details)
iana edited the summary of this revision. (Show Details)
iana marked an inline comment as done.Jun 21 2023, 12:50 PM
iana updated this revision to Diff 533365.Jun 21 2023, 12:50 PM

Add a comment to header_information.py explaining why __iterator/readable_traits.h is a private textual header.

iana edited the summary of this revision. (Show Details)Jun 21 2023, 3:12 PM
ldionne requested changes to this revision.Jun 26 2023, 12:29 PM
ldionne added inline comments.
libcxx/include/__algorithm/iterator_operations.h
23 ↗(On Diff #533365)

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.

libcxx/include/__algorithm/ranges_partial_sort.h
12 ↗(On Diff #533365)

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.

libcxx/include/__algorithm/ranges_partial_sort_copy.h
12 ↗(On Diff #533365)

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.

libcxx/include/__chrono/high_resolution_clock.h
15 ↗(On Diff #533365)

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" }
libcxx/include/__filesystem/path.h
17

This one is definitely correct, it was a missing include.

libcxx/include/__format/format_context.h
24 ↗(On Diff #532438)

Instead, I think locale should export __locale and format_context should only export locale, not export __locale.

libcxx/include/__ranges/all.h
14–15

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.

libcxx/include/__system_error/error_condition.h
15

This is adding a missing include -- obviously correct.

libcxx/utils/libcxx/test/header_information.py
149–152 ↗(On Diff #533365)

Is this still going to be required after we make __iterator/readable_traits.h a standalone header?

This revision now requires changes to proceed.Jun 26 2023, 12:29 PM
ldionne added inline comments.Jun 26 2023, 12:31 PM
libcxx/utils/find_transitive_includes.py
1 ↗(On Diff #533365)

This script doesn't seem to be used in this review, is there any reason why it's being added (or added right now)?

iana updated this revision to Diff 534830.Jun 26 2023, 9:27 PM
iana marked 6 inline comments as done.

Review feedback, split the review into 3

iana added inline comments.Jun 26 2023, 9:44 PM
libcxx/include/cwchar
113

I think it was transitively included, but cwchar explicitly exports the module, so it needs to include the header.

libcxx/include/string_view
230

string_view's module explicitly exports it, and this one I'm pretty sure wasn't being transitively included. So this export would definitely break without the include being added.

iana edited the summary of this revision. (Show Details)Jun 26 2023, 11:03 PM
iana updated this revision to Diff 534844.EditedJun 26 2023, 11:06 PM

Fix the std.concepts.__concepts.equality_comparable in std.iterator.__iterator.concepts

ldionne added inline comments.Jun 28 2023, 6:45 AM
libcxx/include/__memory/concepts.h
22

Why is this one needed?

libcxx/include/cwchar
113

Instead, what happens if we remove export depr.stdio_h from module cwchar? This seems like a better approach to me?

libcxx/include/string_view
230

I think we want to remove the include and also remove the export initializer_list from module string_view. Some headers are specified to include <initializer_list>, but <string_view> isn't. I suspect the export was added incorrectly.

libcxx/include/tgmath.h
27–28

What happens if we remove the exports instead? Our "C compatibility" headers are normally just #include_nexting the C library header, and we should strive to keep them as close as possible to that.

iana marked 9 inline comments as done.Jun 28 2023, 2:10 PM
iana added inline comments.
libcxx/include/cwchar
113

I'll try that with the rest of these. I've been hesitant to remove the exports because their lack causes linker errors post-module-split that are sensitive to exactly what gets included and which templates are used. The CI catches a lot of them, but probably not all. Maybe world building everything at Apple will shake out the rest of them, I was just going for a more conservative approach and keeping what was there working.

libcxx/utils/libcxx/test/header_information.py
149–152 ↗(On Diff #533365)

It will be required in a later change, but not this one anymore.

iana updated this revision to Diff 535636.Jun 28 2023, 11:30 PM
iana marked 2 inline comments as done.

Remove some of the added includes, and instead remove the associated exports

iana updated this revision to Diff 535900.Jun 29 2023, 10:51 AM

The CI fails when __memory/concepts.h doesnt export std.type_traits.remove_reference, so add the include with a comment instead.

ldionne accepted this revision.Jun 29 2023, 11:43 AM

LGTM assuming the CI passes

This revision is now accepted and ready to land.Jun 29 2023, 11:43 AM