This is an archive of the discontinued LLVM Phabricator instance.

[libc++][Modules] Group the private detail headers into larger modules
AbandonedPublic

Authored by iana on Jul 12 2023, 5:22 PM.

Details

Reviewers
ldionne
Mordante
Bigcheese
jdoerfert
Group Reviewers
Restricted Project
Summary

It's not really in the spirit of clang modules to make a top level module for every single header. That's necessary for the public headers, but the private detail headers could be grouped into bigger modules as long as they remain acyclic. Make a Python script to do the grouping.

Diff Detail

Event Timeline

iana created this revision.Jul 12 2023, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2023, 5:22 PM
iana requested review of this revision.Jul 12 2023, 5:22 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
iana planned changes to this revision.Jul 12 2023, 5:22 PM

Need to update this with changes from D144322

iana updated this revision to Diff 539846.Jul 12 2023, 10:26 PM

Update with changes from D155116 and D144322

iana added a comment.EditedJul 12 2023, 10:37 PM

@ldionne is concerned about the maintainability of this patch. The module generation script is admittedly complicated.

  1. validate_exports needs to be updated if module naming conventions change.
  2. write_libcxx_includes needs to be updated when the subdirectories and files that need to be ignored are updated.
  3. build_include_tree needs to be updated when new C++ versions are added.
  4. build_include_tree needs to be updated when new macros control what gets included. _LIBCPP_ENABLE_EXPERIMENTAL and _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY were found while debugging module cycles that the script created.

It's probably not obvious that you might need to edit internal_module_attributes.json.in from time to time as includes change and new private detail headers are added.

While I don't personally think that doing any of these is particularly difficult or onerous, I must admit they aren't obvious. If you don't know you have to do them, the errors are going to be fairly difficult to figure out. Several of them you won't realize you've forgotten them until some time later. e.g. you might not realize that you need to add c++26 until years down the road when the modules become cyclic on that version because the script never knew it needed to take that into consideration.

Nevertheless, I'm posting this review because 1) it produces a much nicer module map than can be done by hand, and 2) it might be necessary down the road if build performance suffers for having too many modules.

iana updated this revision to Diff 539850.Jul 12 2023, 10:49 PM

Add a missing requires

iana updated this revision to Diff 540226.Jul 13 2023, 4:49 PM

Pick up changes from D144322

iana updated this revision to Diff 540506.Jul 14 2023, 11:16 AM

Pick up changes from D144322

iana updated this revision to Diff 540644.Jul 14 2023, 11:14 PM

Remove some dead code from the generator script.

iana updated this revision to Diff 540753.Jul 15 2023, 9:28 PM

Don't allocate a Header for every line from cxx -H, which is what was happening from the dict.setdefault call. Break that up into a get/test/set sequence.

iana updated this revision to Diff 542158.Jul 19 2023, 12:49 PM

Update Contributing.rst

iana updated this revision to Diff 542637.Jul 20 2023, 12:50 PM

Rebase, fix the documentation

iana updated this revision to Diff 542652.Jul 20 2023, 1:34 PM

Really fix the documentation

@iana is this patch still active or is it obsolete with the recent commits?

iana updated this revision to Diff 546977.Aug 3 2023, 12:51 PM

Rebase, pick up changes from D156177 and D156508

[Github PR transition cleanup]

I've said it before but I think this is too complicated and we don't have a clear picture of the benefits. I would prefer abandoning this patch, but if you really want to move forward with it, please provide compelling evidence of why this complexity is worth adding.

iana abandoned this revision.Sep 7 2023, 10:26 PM

[Github PR transition cleanup]

I've said it before but I think this is too complicated and we don't have a clear picture of the benefits. I would prefer abandoning this patch, but if you really want to move forward with it, please provide compelling evidence of why this complexity is worth adding.

I still think this results in a better module map; module-per-header solves our requirements but just feels very inelegant. But I think I need to split out some of the variables into the json file like which standards to run and which macros go with which standards. And probably convert that into a Python file akin to header_info.py. I'm also maybe overestimating how often the module map needs to change, and maybe it could be a regular generated file instead. I'll abandon this for now, and if we end up needing it after llvm 17 ships, or I get a bit of time to work on it and I'm still bummed about all the work I put in on this, I'll try it for review on GitHub.