Page MenuHomePhabricator

[libcxx] adds remaining callable concepts
ClosedPublic

Authored by cjdb on Feb 10 2021, 6:40 PM.

Details

Summary
  • std::predicate
  • std::relation
  • std::equivalence_relation
  • std::strict_weak_order

Implements parts of:

  • P0898R3 Standard Library Concepts
  • P1754 Rename concepts to standard_case for C++20, while we still can

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
cjdb planned changes to this revision.Mar 25 2021, 11:23 AM
cjdb added inline comments.
libcxx/test/std/concepts/callable/equiv.compile.pass.cpp
88 ↗(On Diff #328363)

@zoecarver I'm confused about whether or not you're saying what's present is sufficient, or if you'd like something else.

libcxx/test/std/concepts/callable/functions.h
27–28 ↗(On Diff #328363)

There is no conceivable library bug that could cause us to treat int Value = false; and int Value = 0; any differently.

That appears to have been integrated (I don't recall whether I meant bool Value or int Value, but the point is moot either way).

I was referring to the suggested addition of explicit, which is not desirable, since __boolean_testable requires Bool to be both implicitly and explicitly convertible.

Well, I'm always on the side of "keep it simple/short."

I don't think noexcept changes this in either direction.

Also, it'll rarely if ever be noexcept in real-world code, so one could argue that testing shorter code is also testing more realistic code.

Citation needed. I certainly apply noexcept to everything that's conceivable.

cjdb updated this revision to Diff 334360.Mar 30 2021, 11:41 PM

reboots testing for this patch

Generally looks good!

libcxx/test/std/concepts/concepts.callable/concepts.equiv/equivalence_relation.pass.cpp
17–23 ↗(On Diff #334360)

So bool(...) is always a relation but bool (*)(...) is never a relation? ;)

libcxx/test/std/concepts/concepts.callable/concepts.equiv/equivalence_relation.subsumption.pass.cpp
27 ↗(On Diff #334360)

IIUC, the point of this test is that equivalence_relation<F&, T, U> subsumes relation<F&, T, U>.
Is it also mandated that relation<F&, T, U> subsumes equivalence_relation<F&, T, U>? (I think it is.) If so, let's add a test for that.
I'd like to remove those redundant ampersands at the same time — i.e., change F& to F everywhere it appears, unless the & is significant.

I would also like to see a test that equivalence_relation<F, T, U> [subsumes|does not subsume] equivalence_relation<F, T, T>.

libcxx/test/std/concepts/concepts.callable/concepts.predicate/predicate.pass.cpp
30 ↗(On Diff #334360)

Here I do think it's worth testing std::predicate<int (S::*)(), S&> as well.

42 ↗(On Diff #334360)

Here it's worth testing !std::predicate<const Predicate, int, double, char>.

libcxx/test/std/concepts/concepts.callable/concepts.predicate/predicate.subsumption.pass.cpp
43 ↗(On Diff #334360)

This test confuses me. I think you're saying "a regular-invocable callable whose return value is not convertible to bool, is not a predicate; here is an example." However, that doesn't really have anything to do with subsumption, right? The second overload (lines 21–28) simply drops out of overload resolution entirely because it's not viable. Also, line 41 could be deleted from this test and nothing would change — the existence of that explicit conversion to bool doesn't matter to the test.

I recommend deleting lines 40–43, and just making sure you have the equivalent of
static_assert(!std::predicate<explicit_bool()>) in predicate.pass.cpp. Arguably even better, add the __boolean_testable tests requested by @Mordante, and make sure that !std::__boolean_testable<explicit_bool>. (But that would be a libcxx/-only test, so maybe you'd want to keep the test for !std::predicate<explicit_bool()> as well, for the benefit of people testing non-libc++ libraries against the libc++ test suite.)

libcxx/test/std/concepts/concepts.callable/concepts.relation/relation.subsumption.pass.cpp
24 ↗(On Diff #334360)

Again I'm confused by the 5 redundant &s in this file. I recommend removing them (unless that makes the test fail, in which case I'd want to know why).

Mordante accepted this revision as: Mordante.Mar 31 2021, 11:15 AM

LGTM, approved so @Quuxplusone can give his final approval after addressing his comments.

cjdb updated this revision to Diff 334544.Mar 31 2021, 2:37 PM
cjdb marked 4 inline comments as done.

applies all of @Quuxplusone's feedback (please confirm nothing was missed)

cjdb added inline comments.Mar 31 2021, 2:38 PM
libcxx/test/std/concepts/concepts.callable/concepts.predicate/predicate.pass.cpp
42 ↗(On Diff #334360)

Wow, I didn't even realise I left off the const.

libcxx/test/std/concepts/concepts.callable/concepts.predicate/predicate.subsumption.pass.cpp
43 ↗(On Diff #334360)

However, that doesn't really have anything to do with subsumption, right?

Good point, moved.

Also, line 41 could be deleted from this test and nothing would change — the existence of that explicit conversion to bool doesn't matter to the test.

It does, because the "don't directly test the implementation detail" model of testing I follow requires I check that the result of the invocable has a return type that satisfies convertible_to<bool> (as opposed to something conceptifying just is_convertible_v<bool>). However, ...

Arguably even better, add the __boolean_testable tests requested by @Mordante, and make sure that !std::__boolean_testable<explicit_bool>. (But that would be a libcxx/-only test, so maybe you'd want to keep the test for !std::predicate<explicit_bool()> as well, for the benefit of people testing non-libc++ libraries against the libc++ test suite.)

...I'm not a fan of adding tests for implementation details, but if we've got a history of doing this in libcxx/test/libcxx/, I'll make a separate patch for that before signing off on P0898. @ldionne which would you prefer?

libcxx/test/std/concepts/concepts.callable/concepts.relation/relation.subsumption.pass.cpp
24 ↗(On Diff #334360)

This might be a relic from the Ranges TS. I didn't look too hard, but I can't find any mention of the & in standard wording, but this was a thing way back when. I've deleted them across all files (please check this).

Quuxplusone requested changes to this revision.Apr 1 2021, 4:44 PM
Quuxplusone added inline comments.
libcxx/include/concepts
447

Gratuitous removal of one space character here

libcxx/test/std/concepts/concepts.callable/concepts.equiv/equivalence_relation.subsumption.pass.cpp
22–24 ↗(On Diff #334544)

I don't think you ever use this overload, do you? (Notice that it takes only two template parameters, not three.)

I recommend avoiding overload sets as much as possible (in real life as well as in libc++ tests ;))

53 ↗(On Diff #334544)

This template is never used.

This revision now requires changes to proceed.Apr 1 2021, 4:44 PM
Quuxplusone added inline comments.Apr 1 2021, 4:49 PM
libcxx/test/std/concepts/concepts.callable/concepts.strictweakorder/strict_weak_order.pass.cpp
29 ↗(On Diff #334544)

FWIW, I have no idea what this assertion is testing. It's asking whether you can use a pointer-to-data-member as a callable, passing it an S1* and nullptr, and get back a boolean answer? Isn't the answer obviously "no"?

Vice versa, it occurs to me that

static_assert(std::relation<bool (S::*)(S*), S*, S*>);
static_assert(std::relation<bool (S::*)(S&), S&, S&>);

might be considered non-obvious.

libcxx/test/std/concepts/concepts.callable/concepts.strictweakorder/strict_weak_order.subsumption.pass.cpp
18–24 ↗(On Diff #334544)

Two instances of F& that could be F, here.
(Also, I'd prefer to see you remove the gratuituous clang-format comments; they don't add anything.)

cjdb updated this revision to Diff 334869.Apr 1 2021, 5:30 PM

applies most of @Quuxplusone's feedback

cjdb marked 2 inline comments as done.Apr 1 2021, 5:30 PM
cjdb added inline comments.
libcxx/test/std/concepts/concepts.callable/concepts.strictweakorder/strict_weak_order.pass.cpp
29 ↗(On Diff #334544)

Agreed on both counts, changed in all three files.

libcxx/test/std/concepts/concepts.callable/concepts.strictweakorder/strict_weak_order.subsumption.pass.cpp
18–24 ↗(On Diff #334544)

Two instances of F& that could be F, here.

Thanks for checking, I had a feeling I'd missed a couple.

(Also, I'd prefer to see you remove the gratuituous clang-format comments; they don't add anything.)

On the contrary; without disabling, the formatting becomes

template <class F, class T, class U>
requires std::relation<F, T, U> [[nodiscard]] constexpr bool
check_subsumption() {
  return false;
}

template <class F, class T, class U>
    requires std::strict_weak_order<F, T, U> &&
    true [[nodiscard]] constexpr bool check_subsumption() {
  return true;
}

which is not only unreadable: it's inconsistent.

clang-format doesn't properly understand requires-clauses. Once it does, I'll go back and erase all the clang-format off/clang-format on guards in this corner of testing.

cjdb updated this revision to Diff 334871.Apr 1 2021, 5:31 PM
cjdb marked 2 inline comments as done.

missed a few de-ampersandifications

This LGTM. I like these tests a lot better :)

Only question: why do we need a separate test file for "subsumption" tests? WDYT about merging those into the "main" test file?

libcxx/include/concepts
430

Do we need to update the synopsis with these?

cjdb marked an inline comment as done.Apr 2 2021, 9:57 AM

This LGTM. I like these tests a lot better :)

Only question: why do we need a separate test file for "subsumption" tests? WDYT about merging those into the "main" test file?

I plan to go back at some point and add subsumption tests for all concepts. I think there are quite a few that would benefit from being in their own source files for brevity's sake, and wanna keep that consistent.
Consider this a pilot?

libcxx/include/concepts
430

I added these in the very first <concepts> commit (which is different to how I'm doing things for <iterator> and <ranges>). Unless there's some divergence between synopsis and code that you're pointing out that I'm missing?

cjdb updated this revision to Diff 334986.Apr 2 2021, 10:33 AM
cjdb marked an inline comment as done.

updates status paper

zoecarver accepted this revision as: zoecarver.Apr 2 2021, 11:08 AM

I plan to go back at some point and add subsumption tests for all concepts. I think there are quite a few that would benefit from being in their own source files for brevity's sake, and wanna keep that consistent.
Consider this a pilot?

As discussed offline, I'm OK a) with these tests and b) with doing them in another file.

Just for the sake of posterity: in general, I don't like tests that say "here is the implementation re-written exactly the same way, assert these two implementations look the same." That's what I thought this was at first (and there's an argument to be made that it is still that to a certain degree), but I think it's OK here because the constraints need to be entirely identical. Essentially what we're testing is that std::relation (for example) is implemented using std::predicate and not some helper concept that looks exactly like std::predicate.

I'll let @Quuxplusone give the final approval.

libcxx/include/concepts
430

Ah, missed that. This looks good then.

Line 429/447 still contains a gratuitous whitespace diff.

I suggest fixing that space, then removing the .subsumption.pass.cpp tests from this PR, then landing it.
I think it would be reasonable to take the subsumption tests (with my suggested approach above) into a fresh PR.

libcxx/test/std/concepts/concepts.callable/concept.predicate/predicate.subsumption.pass.cpp
17 ↗(On Diff #334986)

I would strongly prefer to see as much of the "cruft" removed from this test as possible, so that we could have a bunch of subsumption tests. The extreme form of that would be to use the SUBSUMES macro from https://quuxplusone.github.io/blog/2018/09/23/member-concepts-ii/ . But I think we should actually do more like
https://godbolt.org/z/51edrGrM8

bool model(int, int) { return true; }

constexpr bool relation_subsumes_predicate(std::relation auto) requires true { return true; }
constexpr void relation_subsumes_predicate(std::predicate auto) { }
static_assert( relation_subsumes_predicate(model) );

in a single .compile.fail.cpp file, so that we can verify easily also when a subsumption relationship does not exist.

If anyone knows a clever way to detect non-subsumption in a .compile.pass.cpp test, ping me in #meta-programming or something. ;)

cjdb added a comment.EditedApr 2 2021, 12:42 PM

Line 429/447 still contains a gratuitous whitespace diff.

I suggest fixing that space, then removing the .subsumption.pass.cpp tests from this PR, then landing it.
I think it would be reasonable to take the subsumption tests (with my suggested approach above) into a fresh PR.

I don't see why this is blocking.

We should have a discussion on Discord to address this suggested approach because I don't understand why you consider it to be "better", but it can be landed in a future patch at any time if we agree on that path.
I don't like the idea of removing coverage that shows something is working.

libcxx/test/std/concepts/concepts.callable/concept.predicate/predicate.subsumption.pass.cpp
17 ↗(On Diff #334986)

I would strongly prefer to see as much of the "cruft" removed from this test as possible, so that we could have a bunch of subsumption tests.

It's not exactly clear to me what "cruft" you're referring to.

cjdb updated this revision to Diff 335175.Apr 4 2021, 2:27 PM

rebases to activate CI

Quuxplusone added inline comments.Apr 5 2021, 7:29 AM
libcxx/docs/Cxx2aStatusPaperStatus.csv
79

Should this say "13.0"?

libcxx/include/concepts
447

Please take the edit before landing.

libcxx/test/std/concepts/concepts.callable/concept.equiv/equivalence_relation.subsumption.pass.cpp
22–24 ↗(On Diff #335175)

This template-of-two-parameters is never used. Please remove it from this patch.

51–53 ↗(On Diff #335175)

This template check_no_subsumption is never used. Please remove it from this patch.

libcxx/test/std/concepts/concepts.callable/concept.predicate/predicate.subsumption.pass.cpp
17 ↗(On Diff #334986)

It's not exactly clear to me what "cruft" you're referring to.

For the record: // clang-format off, [[nodiscard]], // clang-format on... Basically anything that doesn't appear in the three-line version I posted above. (I'd even prefer to find a way to eliminate constexpr, but I don't think that's feasible.)

cjdb updated this revision to Diff 335274.Apr 5 2021, 9:14 AM
cjdb marked 8 inline comments as done.

changes incorrect version from 14.0 to 13.0 and renames incorrectly-named overload

cjdb added a comment.Apr 5 2021, 9:22 AM

This patch has been open for almost two months now, so if there's nothing else that urgently needs attention, I'd like to see it merged today.

libcxx/test/std/concepts/concepts.callable/concept.equiv/equivalence_relation.subsumption.pass.cpp
22–24 ↗(On Diff #335175)

This was added at your request. Please confirm you understand what you're asking to be removed (or provide a technical argument explaining why it doesn't meet the conditions of your original request), and I'll remove it between libc++ approval and pushing to main.

51–53 ↗(On Diff #335175)

In order for subsumption to be checked, one must have overloads to choose from. I've renamed it to check_reverse_subsumption instead of deleting.

Quuxplusone resigned from this revision.Apr 5 2021, 10:09 AM

As I said on Discord, there's nothing here that can't be "fixed in post." We use version control; it's easy to make changes to what's been committed. So since I'm the only one still reviewing this patch, okay, go ahead, I take no further responsibility for its correctness. If @ldionne or anyone else objects to my resigning as reviewer on this revision, they can step in and review it themselves.

libcxx/test/std/concepts/concepts.callable/concept.equiv/equivalence_relation.subsumption.pass.cpp
22–24 ↗(On Diff #335175)

Look, you agree that this two-argument function template is never a candidate in any of the calls below, and is therefore useless, right?

And I did not request that you add a useless function template that was never a candidate, right? I suggested that you should add a test that "equivalence_relation<F, T, U> [subsumes|does not subsume] equivalence_relation<F, T, T>." You haven't done that.

LGTM with nitpick applied.

Until we enforce the use of clang-format, please consider format-related comments as non-blocking, unless not fixing them really hinders the readability of the code.

libcxx/test/std/concepts/concepts.callable/concept.equiv/equivalence_relation.subsumption.pass.cpp
22 ↗(On Diff #335274)

I agree, I think this can be removed since it's never used (unless I'm missing something).

cjdb updated this revision to Diff 335364.Apr 5 2021, 6:17 PM

corrects self-subsumption test

cjdb added a subscriber: rsmith.Apr 5 2021, 6:19 PM
cjdb added inline comments.
libcxx/test/std/concepts/concepts.callable/concept.equiv/equivalence_relation.subsumption.pass.cpp
22 ↗(On Diff #335274)

I made a mistake here by forgetting to preserve the third type parameter. I think the most appropriate thing to do here is to fix my error, not to remove the test.
Fix applied everywhere (generalisation courtesy of @rsmith).

PTAL.

curdeius added inline comments.Apr 6 2021, 12:19 AM
libcxx/include/concepts
70–72

Please rename this concept in synopsis (it's already renamed in the implementation) to default_initializable. I let you choose in which patch (or non-patch NFC commit) you do it.

curdeius added inline comments.Apr 6 2021, 12:28 AM
libcxx/include/concepts
83–86

Also, concept boolean is not present in the standard. Please remove it from the synopsis.

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

Ship it with nitpicks applied. Feel free to fix @curdeius 's nitpicks in this patch too as fly-by's, those are really small.

libcxx/test/std/concepts/concepts.callable/concept.equiv/equivalence_relation.subsumption.compile.pass.cpp
79

iteself -> itself? There's a few other occurrences of that in other test files.

This revision is now accepted and ready to land.Apr 6 2021, 8:43 AM
This revision was automatically updated to reflect the committed changes.