Page MenuHomePhabricator

[libc++] Header inclusion tests
ClosedPublic

Authored by Quuxplusone on Mar 24 2021, 4:43 PM.

Details

Summary

As mandated by the Standard's various synopses, e.g. [iterator.synopsis].
Searching the TeX source for '#include' is a good way to find all of these mandates.

The new tests are all autogenerated by utils/generate_header_inclusion_tests.py.
I was SHOCKED by how many mandates there are, and how many of them libc++ wasn't conforming with.

The <iterator> --> <concepts> edge is TODO'ed because that's D99041 / D99044.
All the other TODO'ed edges are because one or the other header doesn't exist in libc++ at all, yet (<ranges>, <coroutine>, <syncstream>).

Diff Detail

Event Timeline

Quuxplusone created this revision.Mar 24 2021, 4:43 PM
Quuxplusone requested review of this revision.Mar 24 2021, 4:43 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 24 2021, 4:43 PM

I had generated iterator.inclusions.pass.cpp before realizing it should have been TODO'ed. git rm the old version hanging around.

cjdb requested changes to this revision.Mar 24 2021, 8:40 PM

Overall LGTM.

libcxx/include/chrono
16

Have we added <compare> support for <chrono> yet? If not, I'd prefer to keep it out of the synopsis. (If you're keeping it in, please add // C++20 or something.)

libcxx/test/libcxx/inclusions/algorithm.inclusions.pass.cpp
1 ↗(On Diff #333184)

Can these please be placed under libcxx/test/std/.../${subclause}/${subclause}.syn/ instead? They're responsible for checking the synopsis, so I'd prefer they sit with the other tests of their category.

29 ↗(On Diff #333184)

Missing new line.

This revision now requires changes to proceed.Mar 24 2021, 8:40 PM
curdeius added inline comments.
libcxx/utils/generate_header_inclusion_tests.py
21

A small comment on how to get/update this list would be nice (grepping draft's source TeX for #include and so on).

180

Nit: new line.

193

Depending on what we decide on .pass vs. .compile.pass, these tests are candidates for begin compile.pass tests (with a defensive return 1; in main).
And, as they're generated, there's less risk to make things incorrect (mind that I don't say "no risk" because someone might theoretically add runtime tests in this generator script).

Quuxplusone marked 3 inline comments as done.
Quuxplusone added reviewers: curdeius, Mordante.

Add missing trailing newline.
Fix TEST_STD_VER == 11 -> TEST_STD_VER >= 11. (Anyone got a better "C++0x or later" check?)

Nice to see this patch!

libcxx/utils/generate_header_inclusion_tests.py
172

Wouldn't it be nicer when the script produces properly formatted code?

Add missing trailing newline.
Fix TEST_STD_VER == 11 -> TEST_STD_VER >= 11. (Anyone got a better "C++0x or later" check?)

Why not just TEST_STD_VER > 3? Cf. https://github.com/llvm/llvm-project/blob/main/libcxx/test/support/test_macros.h#L95.
This would remove special casing for v==11.

Quuxplusone marked an inline comment as done and 2 inline comments as not done.Apr 5 2021, 6:58 AM
Quuxplusone added inline comments.
libcxx/include/chrono
16

There's no particular "support" needed; I'm just making libc++ provide the header inclusion that is mandated by the Standard.

...On second glance, I guess you're objecting only to the comment on line 16, and not to the functional conformance improvement on line 830. Is that right? I have no strong objection to removing line 16 (and likewise throughout the comments), and am inclined to just do it. It'd shrink the diff, too!
Anyone else care to advocate for keeping these new synopsis comments?

libcxx/test/libcxx/inclusions/algorithm.inclusions.pass.cpp
1 ↗(On Diff #333184)

Unfortunately there's no way to do that, without some human coming up with the ad-hoc list of subclauses and creating all those directories. And then we'd have the autogenerated tests scattered all over the codebase, which I foresee leading to problems as people add and remove autogenerated tests. So, no.

libcxx/utils/generate_header_inclusion_tests.py
172

Yes-ish, but my impression is we can't convince clang-format to accept this code.
Anyway, I just copy-pasted this line from the other two generator scripts.

Mordante added inline comments.Apr 5 2021, 7:12 AM
libcxx/utils/generate_header_inclusion_tests.py
172

Oke then let's keep it for now. Maybe once we tune clang-format further it will be possible to remove this line.

Quuxplusone marked an inline comment as done and an inline comment as not done.

Remove synopsis comments per cjdb's feedback.

Script improvements. Note that 03 is an octal constant; but I'm hoping nothing in our system complains about that. I didn't try very hard to find a way of printing it as an integer while still allowing us to use (and string-compare) strings like "2b" in the script.

Quuxplusone marked 2 inline comments as done.Apr 5 2021, 7:15 AM

Rebase and update for D99041. <iterator> can now safely include <concepts>.
Also, poke buildbot, because it seems to be stuck.

curdeius accepted this revision as: curdeius.Apr 6 2021, 12:47 AM

LGTM % nit and the pass/compile.pass naming decision.

libcxx/utils/generate_header_inclusion_tests.py
115–116

Nit: remove modern_.

Quuxplusone accepted this revision.Apr 6 2021, 8:19 AM

Any further comments from @cjdb? I plan to land this (with the modern_ removed) in a few hours.

ldionne accepted this revision.Apr 6 2021, 8:56 AM

This LGTM with some comments. Huge thanks for writing this. You don't have to re-review if you apply my comments and other reviewer's outstanding comments:

  • The section on Adding a new header TODO should be updated in Contributing.rst.
  • Use .compile.pass.cpp BUT don't define main() - this way there's no possibility for confusion on whether the test should run or not.
This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2021, 12:32 PM
This revision was automatically updated to reflect the committed changes.