This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add missing includes
ClosedPublic

Authored by ldionne on Jun 28 2023, 7:22 AM.

Details

Reviewers
Mordante
Group Reviewers
Restricted Project
Commits
rG60a6a0d17ad5: [libc++] Add missing includes
Summary

Those were found while trying to enable configurations like no-threads
and no-localization with Clang modules enabled.

Diff Detail

Event Timeline

ldionne created this revision.Jun 28 2023, 7:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 7:22 AM
ldionne requested review of this revision.Jun 28 2023, 7:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2023, 7:22 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision.Jun 28 2023, 8:29 AM
Mordante added a subscriber: Mordante.

Can you update the commit message by specifying which modules, I assume the clang ones, but the changes to libcxx/test/std/utilities/format/types.compile.pass.cpp suggest the std module.

LGTM when the CI passes.

libcxx/include/regex
801

According the the CI at least this header is not in alphabetic order.

libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp
23–27

This is now in alphabetic order, in .clang-format we explicitly disable that for the tests. Any objections when I make a patch to remove that from that file. Then new tests are automatically formatted with headers sorted.

This revision is now accepted and ready to land.Jun 28 2023, 8:29 AM
ldionne marked 2 inline comments as done.Jun 29 2023, 8:01 AM

Can you update the commit message by specifying which modules, I assume the clang ones, but the changes to libcxx/test/std/utilities/format/types.compile.pass.cpp suggest the std module.

LGTM when the CI passes.

This was for Clang modules. I updated the commit message!

libcxx/include/regex
801

Thanks -- apparently VS Code disagrees with every other tool about what it means to be sorted.

libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp
23–27

I don't think I'd have an objection, however I think you might hit some actual blockers while trying to do that? We probably have tests where the order of includes is important?

In that case I guess we could always use // clang-format on/off around the #includes.

ldionne updated this revision to Diff 535807.Jun 29 2023, 8:03 AM
ldionne marked 2 inline comments as done.
ldionne edited the summary of this revision. (Show Details)

Fix sorted includes

Thanks for updating the message!

libcxx/include/regex
801

Maybe is uses locales ;-)

libcxx/test/std/input.output/file.streams/fstreams/ifstream.cons/path.pass.cpp
23–27

Actually I think that is a good reason *not* to do it. Then formatting in the future may accidentally "break" a test. It still would be good when test call out the order is important.

ldionne updated this revision to Diff 535973.Jun 29 2023, 1:13 PM

Adjust transitive includes.

EricWF added a subscriber: EricWF.Jun 29 2023, 1:16 PM

Nit, this is definitely not NFC, because why else would we do it?
I think we should use NFC less because it's rarely true.

ldionne retitled this revision from [libc++][NFC] Add missing includes to [libc++] Add missing includes.Jun 29 2023, 1:51 PM

Nit, this is definitely not NFC, because why else would we do it?
I think we should use NFC less because it's rarely true.

You're right, I'll update the commit message upon landing.

This revision was automatically updated to reflect the committed changes.