Page MenuHomePhabricator

[libc++] first steps of a private modulemap
Needs ReviewPublic

Authored by tschuett on Jan 18 2021, 11:01 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

Note that __config is not a module anymore.

Note that there are a lot of cyclic dependencies hidden by the main module.

Diff Detail

Event Timeline

tschuett created this revision.Jan 18 2021, 11:01 AM
tschuett requested review of this revision.Jan 18 2021, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2021, 11:01 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
tschuett edited the summary of this revision. (Show Details)
tschuett updated this revision to Diff 317405.Jan 18 2021, 12:10 PM

add newline

tschuett updated this revision to Diff 317415.Jan 18 2021, 1:36 PM

more pseudo-private modules

tschuett updated this revision to Diff 317423.Jan 18 2021, 2:51 PM

finally managed to get __bits running

tschuett edited the summary of this revision. (Show Details)Jan 18 2021, 2:51 PM
tschuett edited the summary of this revision. (Show Details)
231084fg removed a reviewer: Restricted Project.Jan 18 2021, 6:23 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 18 2021, 6:23 PM
ldionne requested changes to this revision.Jan 19 2021, 11:59 AM
ldionne added a subscriber: ldionne.

Is this ready for review? If not, please ping me when it is. In the meantime, I'll "request changes" so it shows up correctly in the queue.

libcxx/include/__undef_macros_private
13

Can you please indent nested preprocessor directives?

libcxx/include/module.modulemap
4

Why is this not an issue anymore? We still reference this module below in module std, don't we?

This revision now requires changes to proceed.Jan 19 2021, 11:59 AM

You can see it as proof of concept. If you don't like, feel free to reject.

libcxx/include/__undef_macros_private
13

It should be a plain copy of__undef_macros?!?

libcxx/include/module.modulemap
4

The problem is that there are two modules in module.modulemap. Private modules don't work with having two modules.

tschuett added inline comments.Jan 19 2021, 12:33 PM
libcxx/include/__undef_macros_private
3

undef_macros_private is an exact copy of undef_macros. I didn't even change the name.

Generally speaking, I have no objection with doing this if it's an improvement. But I don't understand the benefits / tradeoffs here cause I don't have much experience with Clang modules (so far the libc++ Clang modules are kind of a side thing maintained by people who care about it, but we still use the non-modular build by default). Can you explain what introducing private modules is going to buy us, and what sort of changes are going to be required? Will this have any visible change for people building with -fcxx-modules or -fmodules today?

libcxx/include/__undef_macros_private
13

Wait, so why do we keep both __undef_macros and __undef_macros_private? Sorry if that's a basic question, I'm not very familiar with private module maps.

  1. You don't have to do anything.
  2. If you use -fcxx-modules, it reduces the size of the surface of the std module. The std_private module is hidden from users. There is a " // FIXME: These should be private." in the module.modulemap. These headers should not be visible to users of the std module. The private module is exactly trying to do that.
libcxx/include/__undef_macros_private
13

Unfortunately, we have to keep both. Otherwise we get cyclic dependencies between the two modules (std and std_private).

  1. You don't have to do anything.
  2. If you use -fcxx-modules, it reduces the size of the surface of the std module. The std_private module is hidden from users. There is a " // FIXME: These should be private." in the module.modulemap. These headers should not be visible to users of the std module. The private module is exactly trying to do that.

Nice! Then yes, I think it's a good idea to do this. It all depends on what changes are needed of course, but I'd be favorable if the changes are reasonable. It definitely seems like an improvement to me.

libcxx/include/__undef_macros_private
13

Ugh! Are there other base headers that we'd have to duplicate this way, too? If there's no other solution but to duplicate, that would be a serious issue from my point of view. Duplicating this simple header is not a deal breaker, but anything more complex might be.

curdeius added inline comments.
libcxx/include/module.modulemap
525

Is this order change intended? It was alphabetically sorted before...

tschuett updated this revision to Diff 318448.Jan 22 2021, 12:57 AM
  • rebased
  • sorted headers
tschuett marked an inline comment as done.Jan 22 2021, 12:58 AM

I would rather expect that we have to split some headers into smaller/modular ones.

I would rather expect that we have to split some headers into smaller/modular ones.

Yes, indeed. I would 100% support that. In fact, I've started doing that here and there when it makes sense. If you want to pursue such an effort, I would definitely back it up.

I would rather see it as that I am forced to do that. It is the only way to eliminate cyclic dependencies.