This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Ensure that all public C++ headers include <__assert>
ClosedPublic

Authored by ldionne on Mar 25 2022, 1:56 PM.

Details

Summary

This patch changes the requirement for getting the declaration of the
assertion handler from including <__assert> to including any public
C++ header of the library. Note that C compatibility headers are
excluded because we don't implement all the C headers ourselves --
some of them are taken straight from the C library, like assert.h.

It also adds a generated test to check it. Furthermore, this new
generated test is designed in a way that will make it possible to
replace almost all the existing test-generation scripts with this
system in upcoming patches.

Diff Detail

Event Timeline

ldionne created this revision.Mar 25 2022, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 1:56 PM
ldionne requested review of this revision.Mar 25 2022, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 1:56 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 418690.Mar 28 2022, 1:22 PM
ldionne retitled this revision from [libc++] Ensure that all public headers include <__assert> to [libc++] Ensure that all public C++ headers include <__assert>.
ldionne edited the summary of this revision. (Show Details)

Address CI failures.

The assertion handler won't be available after including C compatibility headers, because it's really difficult to provide that guarantee without implementing more C headers than we do today. In practice, I don't think this should matter since users can include any other header they like.

ldionne updated this revision to Diff 418842.Mar 29 2022, 5:13 AM

Fix modules issue.

ldionne updated this revision to Diff 418897.Mar 29 2022, 8:59 AM

Hopefully fix last CI issues.

In general quite happy. I think we can do some more minor improvement.

libcxx/include/ext/hash_map
207

Just curious did this fail to compile?

libcxx/include/filesystem
12

Please change to one blank line.

libcxx/include/version
196

I wonder when we include the __assert here, how do you feel about requiring version in every header instead?
When not, maybe extend the script to also always validate version is included? Looking at the number of proposals adding this header, soon every header will require them.

libcxx/utils/generate_assertion_tests.py
11

This code feels duplicated. I don't care too strongly about it, but when we keep duplicating it more times we maybe should look at having one source of truth for these lists.

21

I think this restriction isn't required it's safe to include this header when format is disabled.

33

I think this restriction isn't required it's safe to include this header when ranges are disabled.

45

I think this restriction isn't required it's safe to include this header when coroutines are disabled.

57

Except for experimental/regex I don't see an #error in these headers. Do we need them in this list?

ldionne updated this revision to Diff 419114.Mar 30 2022, 5:25 AM
ldionne marked 8 inline comments as done.

Address review comments!

libcxx/include/ext/hash_map
207

Yeah, this started failing to compile, I think it's because previously we never included ext/hash_map alone in a file, we always had other headers, and <algorithm> was probably always included before ext/hash_map.

libcxx/include/version
196

I'd be fine with that, although we most likely want to include <version> in all C++ headers only (not C compatibility headers, for the same reason as here). I volunteer to do it in a separate patch.

libcxx/utils/generate_assertion_tests.py
11

I fully agree -- maybe it wasn't clear, but I have a follow-on patch locally that removes all the other header generation scripts.

57

We do, because they use C++03-unfriendly constructs in the headers. That's why they were guarded similarly in previous header-generation tests, too.

Mordante accepted this revision.Mar 30 2022, 8:58 AM

LGTM! I've some replies but they require no actions.

libcxx/include/version
196

Thanks for volunteering!

libcxx/utils/generate_assertion_tests.py
11

That wasn't clear, but sounds good!

This revision is now accepted and ready to land.Mar 30 2022, 8:58 AM