This is an archive of the discontinued LLVM Phabricator instance.

[libc++][random] Removes transitive includes.
ClosedPublic

Authored by Mordante on Sep 3 2022, 4:38 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG24e1736d84fd: [libc++][random] Removes transitive includes.
Summary

It seems these includes are still provided by the sub headers, so it only
removes the duplicates.

There is no change in the list of includes, but the change affects the
modular build. By not having the includes in the top-level header the
module map has changed. This uncovers missing includes in the tests
and missing exports in the module map. This causes the huge amount of
changes in the patch.

Diff Detail

Event Timeline

Mordante created this revision.Sep 3 2022, 4:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2022, 4:38 AM
Mordante requested review of this revision.Sep 3 2022, 4:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2022, 4:38 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 457802.Sep 3 2022, 10:25 AM

Fixes CI.

Mordante updated this revision to Diff 457851.Sep 4 2022, 4:44 AM

Fixed modular build.

Mordante retitled this revision from [NFC][libc++][random] Removes transitive includes. to [libc++][random] Removes transitive includes..Sep 4 2022, 5:04 AM
Mordante edited the summary of this revision. (Show Details)
EricWF added a subscriber: EricWF.Sep 6 2022, 6:59 AM

Who is asking for these changes?

It's gunna break a lot of code, so we better have a rock solid, user-provided, justification for doing it.

Who is asking for these changes?

It's gunna break a lot of code, so we better have a rock solid, user-provided, justification for doing it.

Thanks for your concern.

There was no request, but a set of TODOs I cleaned up.

I want to discuss this with @ldionne since this change is special and not in a positive way.
Before and after the change exactly the same set of headers is included, therefore I marked the change NFC originally.
But not including them in random itself breaks the modular build.

Therefore I'm not convinced we should do this, but at least the test changes are an improvement and if we don't do it we need to remove these TODOs in random and add a warning instead.

ldionne accepted this revision.Sep 6 2022, 9:06 AM
ldionne added a subscriber: philnik.

This change is a natural step in our ongoing effort to clean up incidental transitive includes. It just happened to surface the fact that our modulemap is written in an extremely lazy way (by using export *), which leads to our headers sometimes providing stuff implicitly when they shouldn't.

To be clear:

  • I don't think this will break any more code than the other incidental-transitive-includes removals we've been doing (and which are guarded behind the standard version for now). Specifically, this one is a NFC for users that don't build with modules enabled, and it is a potential breaking change for users that build with -fmodules -std=c++23. I think it's reasonable to expect that those users can fix their includes, that's likely one of the reason they are using modules in the first place.
  • Just to reiterate the motivation for this whole effort, removing transitive includes is necessary to implement some C++ features (otherwise we run into circular dependencies), and it also leads to shorter compilation times as @philnik measured.

IMO what we should do to tackle this issue once and for all is to fix our modulemap so that each header properly exports what it should be exporting, and nothing more. However, I think that is likely a ton of work, it's not fun work, and it's not clear that the ROI is that great given C++20 modules, so perhaps we should not actually do it, and only fix our modulemap as issues come up.

This revision is now accepted and ready to land.Sep 6 2022, 9:06 AM

This change is a natural step in our ongoing effort to clean up incidental transitive includes. It just happened to surface the fact that our modulemap is written in an extremely lazy way (by using export *), which leads to our headers sometimes providing stuff implicitly when they shouldn't.

To be clear:

  • I don't think this will break any more code than the other incidental-transitive-includes removals we've been doing (and which are guarded behind the standard version for now). Specifically, this one is a NFC for users that don't build with modules enabled, and it is a potential breaking change for users that build with -fmodules -std=c++23. I think it's reasonable to expect that those users can fix their includes, that's likely one of the reason they are using modules in the first place.
  • Just to reiterate the motivation for this whole effort, removing transitive includes is necessary to implement some C++ features (otherwise we run into circular dependencies), and it also leads to shorter compilation times as @philnik measured.

IMO what we should do to tackle this issue once and for all is to fix our modulemap so that each header properly exports what it should be exporting, and nothing more. However, I think that is likely a ton of work, it's not fun work, and it's not clear that the ROI is that great given C++20 modules, so perhaps we should not actually do it, and only fix our modulemap as issues come up.

The modulemap works actually pretty well with the granularized includes. With the current style everything defined inside a header in __public_header/ gets re-exported and anything else doesn't. So IMO we should just continue with granularizing all the headers instead of making the modulemap super complex.

libcxx/include/module.modulemap.in
943

Why do you have to re-export vector?

Mordante marked an inline comment as done.Sep 7 2022, 9:46 AM
Mordante added inline comments.
libcxx/include/module.modulemap.in
943

A function in this header returns a vector. So it needs to be exported.
(Before it was exported by the export *.)

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.