This is an archive of the discontinued LLVM Phabricator instance.

[libc++][Modules] Generate the __std_clang_module header
ClosedPublic

Authored by iana on Aug 7 2023, 11:26 PM.

Details

Summary

Use header_information to generate the __std_clang_module header. Instead of using lit_header_restrictions like the manually written header did, make a new header_include_requirements to codify what can be included rather than what can be fully tested.

Diff Detail

Event Timeline

iana created this revision.Aug 7 2023, 11:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2023, 11:26 PM
iana requested review of this revision.Aug 7 2023, 11:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2023, 11:26 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for working on this! I see several not related changes, for now let's keep them but land them separately.

I would like to see a slightly different design before reviewing in-depth.

libcxx/docs/Contributing.rst
55 ↗(On Diff #548075)

Typically our generated files are stored in git and manually regenerated. This makes it easier to track changes.

The 'generated output` CI job regenerated the file and complains when a difference happens.

iana added inline comments.Aug 8 2023, 12:40 PM
libcxx/docs/Contributing.rst
55 ↗(On Diff #548075)

Are you saying this should move to the libcxx-generate-files target? Or it should be a static file that we have to remember to manually regenerate when we add/remove public headers?

iana added a comment.Aug 8 2023, 12:48 PM

I see several not related changes, for now let's keep them but land them separately.

Sorry, which changes should be split?

Mordante added inline comments.Aug 9 2023, 9:47 AM
libcxx/docs/Contributing.rst
55 ↗(On Diff #548075)

It should be generated by libcxx-generate-files and the output should be committed in the CI. Then when you forget to regenerate the file the CI will warn you. That way the file is available in the release tarball and we are sure it's kept up-to-date.

iana added inline comments.Aug 9 2023, 10:10 AM
libcxx/docs/Contributing.rst
55 ↗(On Diff #548075)

Alright, I'll get that done, thanks.

iana updated this revision to Diff 548763.Aug 9 2023, 1:56 PM
iana marked 2 inline comments as done.

Move the generator script over to libcxx-generate-files

iana updated this revision to Diff 548767.Aug 9 2023, 2:10 PM

Revert lint_headers.sh.py

iana updated this revision to Diff 548769.Aug 9 2023, 2:13 PM

Run black over the Python script

iana updated this revision to Diff 548798.Aug 9 2023, 3:51 PM

Rebase

Thanks for working on this! In general happy with the changes, but I like to see the CI green before approving.

libcxx/test/libcxx/assertions/headers_declare_verbose_abort.gen.py
17 ↗(On Diff #548798)

I would like to see this change and its related changes in a separate review.

libcxx/utils/CMakeLists.txt
7
libcxx/utils/generate_std_clang_module_header.py
2

Please add a copyright block.

29

Similar warnings are used in other generated headers.

48

The format script complains about the sorting, mainly hunks like

-#include <limits>
 #include <limits.h>
+#include <limits>

I suspect this is something what happens on Apple where the ordering of . and '>` differs. I know that @ldionne has seen this too. Would it help to force the C locale? This is something we use in other places where we sort data in the CI.

49–51

I feel this is a bit more readable, can you look at using f-strings at other places too.

libcxx/utils/libcxx/header_information.py
17–20

I would like to see these two changes committed separately. When the CI is green feel free to commit these two.

iana added inline comments.Aug 10 2023, 10:11 AM
libcxx/utils/CMakeLists.txt
7

But the one above says Generate the <version> header

Mordante added inline comments.Aug 10 2023, 10:14 AM
libcxx/utils/CMakeLists.txt
7

I missed that. Then let's keep it as is.

iana marked 3 inline comments as done.Aug 10 2023, 11:24 AM
iana added inline comments.
libcxx/test/libcxx/assertions/headers_declare_verbose_abort.gen.py
17 ↗(On Diff #548798)
libcxx/utils/libcxx/header_information.py
17–20

I put them in https://reviews.llvm.org/D157639, is that ok or should I make one more review?

iana marked 5 inline comments as done.Aug 10 2023, 11:48 AM
iana added inline comments.
libcxx/utils/generate_std_clang_module_header.py
48

It's because it's only sorting the header names, i.e. it has "limits.h" and "limits", it doesn't have "<limits.h>" and "<limits>".

49–51

Sure

iana updated this revision to Diff 549124.Aug 10 2023, 11:48 AM
iana marked 2 inline comments as done.

Addressed review comments

iana updated this revision to Diff 549128.Aug 10 2023, 11:54 AM

Ran black over the Python sources again

iana updated this revision to Diff 549237.Aug 10 2023, 9:13 PM

Fix the extra newline at the end of the header

iana added inline comments.Aug 10 2023, 9:15 PM
libcxx/utils/libcxx/header_information.py
61

The module map requires and lit_header_restrictions looked like there were going to be headers that had multiple requirements, which is why the key is requirement tuples. But it turned out that all the headers only have one requirement. Should I keep the tuple keys and support for multiples in the script, or just make the keys strings?

Mordante accepted this revision.Aug 11 2023, 9:12 AM

Thanks for the improvement, this really helps to reduce the maintenance when adding new headers. We don't do it often so more automation is really nice to have.

LGTM!

libcxx/utils/generate_std_clang_module_header.py
60

Great comment, thanks!

This revision is now accepted and ready to land.Aug 11 2023, 9:12 AM
iana marked 2 inline comments as done.Aug 11 2023, 12:49 PM
This revision was landed with ongoing or failed builds.Aug 14 2023, 12:08 PM
This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.Aug 14 2023, 12:12 PM
libcxx/utils/libcxx/header_information.py
61

My preference would be to reinstate something like what we had before https://reviews.llvm.org/D151893.

iana added inline comments.Aug 14 2023, 12:58 PM
libcxx/utils/libcxx/header_information.py
61

Sorry for landing so quickly, I'll redo this one and make another PR.