This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][test] Verify customization point object properties
ClosedPublic

Authored by jloser on Dec 11 2021, 4:49 PM.

Details

Summary

Add test for various customization point object properties as defined by
the Standard. Test various CPOs from <ranges>, <iterator>,
<concepts>, etc.

The test is mostly from https://reviews.llvm.org/D107036 and split up
into this - thanks Arthur!

Diff Detail

Event Timeline

jloser requested review of this revision.Dec 11 2021, 4:49 PM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2021, 4:49 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
jloser updated this revision to Diff 393710.Dec 11 2021, 4:49 PM

Fix formatting in customization_point_object.h

Quuxplusone requested changes to this revision.Dec 11 2021, 5:25 PM

Seems generally commendable, and will save me from having to touch D107036 again — in fact I'll go abandon it now. :)

Take a look at my test in D107036, though — particularly lines 32–35. I think this PR can and should do something like those std::invocable checks.

I have no particular opinion on whether these tests should be separated under all the different Standard sections like you did, or massed together like I did. The only real advantage of massing them together is that it saves a lot of duplication. Someone will have to add a test for each new CPO/niebloid either way, and either way might make it harder or easier to forget to do that, depending on your personal git-grepping style.

libcxx/test/std/ranges/range.access/cpo.compile.pass.cpp
22 ↗(On Diff #393710)

Also std::ranges::ssize; rbegin etc; cdata. Please triple-check all these long lists to make sure you didn't miss anything.

Please either alphabetize, or match cppreference's order, or something. The ordering of lines 16–19 is throwing me off my game. ;)

libcxx/test/support/customization_point_object.h
19 ↗(On Diff #393710)

I think it's safe to eliminate the || is_union_v<T> here, in the name of sanity. :) Officially these things aren't even guaranteed not to be functions, right; but in practice they're 100% definitely classes. Which also means they're 100% definitely not unions.
Personally I'd also eliminate the __is_literal_type(T) check because it doesn't smell portable, but you're certainly right that these things need to be constexpr-time-initializable.
In D10736, my line 22 checks not only that the CPO type is literal, but more specifically that it is constexpr-copy-constructible (and you could add assignment/swap/whatever to that list, if you wanted). To do that kind of thing you need a constexpr function, not a concept.

This revision now requires changes to proceed.Dec 11 2021, 5:25 PM
jloser updated this revision to Diff 393713.Dec 11 2021, 5:34 PM

Remove union check in customization_point_object definition
Update list of types for range.access. Mark several as TODO as they're not implemented yet. Match the ordering as defined on cppreference.

ldionne requested changes to this revision.Dec 13 2021, 6:39 AM

I like the addition of the concept, but why not add the tests to the files where we already test those CPOs? It could become just a section of that test, no?

This revision now requires changes to proceed.Dec 13 2021, 6:39 AM

Generally LGTM, but consider adding the new tests either to existing test files or to a single new test file devoted to verifying customization points.

jloser added a comment.EditedDec 14 2021, 6:26 PM

I think I messed up updating this patch. Instead, it created a new revision at https://reviews.llvm.org/D115777 - sorry about that. I've addressed the feedback and decided to devote all the CPOs from the various headers (<concepts>, <iterator>, <ranges>, etc.) into one test file in that new PR. Thanks for pointing me at your tests from https://reviews.llvm.org/D107036, @Quuxplusone!

I just force updated this revision, so I'll abandon the accidentally-opened-new one.

jloser updated this revision to Diff 394448.Dec 14 2021, 6:30 PM

Force update

jloser updated this revision to Diff 394453.Dec 14 2021, 6:58 PM

Remove extra newline

Personally I quite liked the approach where we'd have a helper to check that a function object is a CPO and then we'd check it from each individual CPO test, but this is fine too. IMO this is just a bit easier to forget to update.

libcxx/test/std/library/description/conventions/customization.point.object/cpo.pass.cpp
42

This main() can be removed, and the test can be made a .compile.pass.cpp test instead.

Personally I quite liked the approach where we'd have a helper to check that a function object is a CPO and then we'd check it from each individual CPO test, but this is fine too. IMO this is just a bit easier to forget to update.

I may slightly prefer having the helper and just use it each test, but don't feel strongly. Arguably, for a "new" (not-yet-implemented CPO like the ones commented out here), it's just as easy to forget to call test_cpo (or whatever we call the helper function template). For the existing, but not-yet-implemented CPOs, this one file approach at least is a gentle TODO for "uncomment me when it's implemented". But, you need to know about the file in the first place from git-grepping around.

Thoughts @Quuxplusone @var-const or others?

Personally I quite liked the approach where we'd have a helper to check that a function object is a CPO and then we'd check it from each individual CPO test, but this is fine too. IMO this is just a bit easier to forget to update.

I may slightly prefer having the helper and just use it each test, but don't feel strongly. Arguably, for a "new" (not-yet-implemented CPO like the ones commented out here), it's just as easy to forget to call test_cpo (or whatever we call the helper function template). For the existing, but not-yet-implemented CPOs, this one file approach at least is a gentle TODO for "uncomment me when it's implemented". But, you need to know about the file in the first place from git-grepping around.

When writing new CPOs, one rarely start from scratch. I think most people will look at the tests for existing CPOs and base their new tests on that. If they do it, then this test_cpo would nicely be discoverable.

jloser updated this revision to Diff 394663.Dec 15 2021, 2:38 PM

Rename to .compile.pass.cpp

jloser updated this revision to Diff 394664.Dec 15 2021, 2:39 PM

Fix spacing

Mainly I'm ambivalent:

I have no particular opinion on whether these tests should be separated [...] or massed together [...E]ither way might make it harder or easier to forget to do that, depending on your personal git-grepping style.

But, another advantage of the massed-together approach is that we can go ahead and add commented-out lines for all the unimplemented CPOs (as @jloser has done). This gives a "TODO" list that is more explicit rather than implicit. Which is good.

I may slightly prefer having the helper and just use it each test, but don't feel strongly. Arguably, for a "new" (not-yet-implemented CPO like the ones commented out here), it's just as easy to forget to call test_cpo (or whatever we call the helper function template). For the existing, but not-yet-implemented CPOs, this one file approach at least is a gentle TODO for "uncomment me when it's implemented". But, you need to know about the file in the first place from git-grepping around.

Thoughts @Quuxplusone @var-const or others?

FWIW, I have a slight preference to the current single-file approach; from the readability perspective, I find "test for common behavior across a range of related types" to be a more natural structuring than "test a single type for a wide range of different unrelated behaviors".

From the discoverability perspective, the one-file-per-class approach is probably slightly better because someone writing a new test will likely base it on an existing test. OTOH, verifying that the tests cover all existing CPOs is much easier with the single-file approach.

jloser updated this revision to Diff 394730.Dec 15 2021, 7:42 PM

Split test_cpo use into their respective individual files.

Move iter_swap.pass.cpp into iterator.cust.swap directory to mirror its iter_move counterpart. Test the cpo there.

We seem to be pretty split on the two approaches. I just updated the patch to see what others things of the approach of testing the CPO in each file. Let me know what you think.

Still have the one-file approach sitting on a branch and we can stick with that instead if desired.

ldionne accepted this revision.Dec 17 2021, 7:42 AM

This is fine, the other approach was fine too. I have a preference for the current one, but my comment was non-blocking so feel free to use whichever approach you prefer.

jloser updated this revision to Diff 395245.Dec 17 2021, 5:42 PM

Back to single cpo.compile.pass.cpp as discussed offline with ldionne. If CI passes still, I'll land this.

ldionne accepted this revision.Dec 20 2021, 8:18 AM

The CI failure seems to be a fluke. LGTM

This revision was not accepted when it landed; it landed in state Needs Review.Dec 21 2021, 7:13 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.