This is an archive of the discontinued LLVM Phabricator instance.

[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

cjdb requested review of this revision.Feb 10 2021, 6:40 PM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 6:40 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Nits only for the moment.

libcxx/test/std/concepts/callable/functions.h
8–12

LAND_ should be removed, nope?

libcxx/test/std/concepts/callable/predicate.compile.pass.cpp
96

Nit: newline.

libcxx/test/std/concepts/callable/strictweakorder.compile.pass.cpp
104

Newline.

cjdb marked 2 inline comments as done.Feb 11 2021, 11:39 AM
cjdb added inline comments.
libcxx/test/std/concepts/callable/functions.h
8–12

See @ldionne's request in D96230.

libcxx/test/std/concepts/callable/predicate.compile.pass.cpp
96

Done, but not pushing to remote since it's not worth activating CI for this.

libcxx/test/std/concepts/callable/strictweakorder.compile.pass.cpp
104

Done, as above.

curdeius added inline comments.Feb 11 2021, 12:01 PM
libcxx/test/std/concepts/callable/functions.h
8–12

You mean this https://reviews.llvm.org/D96230#inline-902629? There was "lang" in the path and a typo in the guard macro.

cjdb marked 2 inline comments as done.Feb 11 2021, 12:24 PM
cjdb added inline comments.
libcxx/test/std/concepts/callable/functions.h
8–12

Ah, good point. I'll fix both up, but won't commit since it's not worth impacting CI times for more important patches.

Mordante added inline comments.Feb 21 2021, 7:21 AM
libcxx/include/concepts
293

I'm somewhat surprised the concept is define like this in the Standard, but http://eel.is/c++draft/concept.booleantestable#2 has additional constrains.
Did I miss the tests for this concept? I'd like some tests, https://en.cppreference.com/w/cpp/concepts/boolean-testable has some examples of valid types: bool, std::true_type, std::bitset<N>::reference, and int*.

libcxx/test/std/concepts/callable/equiv.compile.pass.cpp
14

Copy-paste.

libcxx/test/std/concepts/callable/functions.h
62

The .Value = isn't required right?I think this makes it harder to read.

libcxx/test/std/concepts/callable/strictweakorder.compile.pass.cpp
14

Copy-paste.

cjdb updated this revision to Diff 325525.Feb 22 2021, 11:53 AM
cjdb marked 3 inline comments as done.

addresses comments by @Mordante and @curdeius

cjdb updated this revision to Diff 328363.Mar 4 2021, 7:41 PM

applies final feedback from @Mordante

libcxx/include/concepts
293

It's an implementation detail, so I'm not motivated to directly test it. All the predicate-based concepts are tested for bool, Bool, and int, but I've expand to include std::true_type and int*.

cjdb edited subscribers, added: CaseyCarter, mclow.lists; removed: curdeius.
cjdb added a comment.Mar 12 2021, 8:58 AM

Ping. This one's been open for a long while now.

libcxx/include/concepts
253

Moving copy_constructible upward is a severable change you can just land right away without review, if you want to. (IMHO. I mean if it were me, I'd land it. Maybe someone else will chime in to agree/disagree.)

314

You changed this comment, but line 152 implies that you shouldn't have. Also, insert blank line.

libcxx/test/std/concepts/callable/equiv.compile.pass.cpp
25

Please indent the requires line by one indent-level (which I guess is 2 spaces, in the style used by this file, blecch).
However, as I say below, I'd rather eliminate these functions altogether...

29
51

I'd prefer to see

static_assert(!std::equivalence_relation<decltype(&A::F), int, int>);

It's not significantly harder to read or reason about, and I think it's a little simpler when it comes to testing lvalue parameter types (which are woefully absent from the current tests). That is, I'd like to see at least one instance of

static_assert(!std::equivalence_relation<decltype(&A::F), int&, int&>);

In fact, I'd like to see a test case where

static_assert(std::equivalence_relation<X, int, int>);
static_assert(!std::equivalence_relation<X, int&, int&>);

and another test case where

static_assert(!std::equivalence_relation<X, int, int>);
static_assert(std::equivalence_relation<X, int&, int&>);
88

I don't think it's useful to keep these as comments.
What is your understanding of the requirements on libc++ as a conforming implementation of C++20? Is libc++ permitted to reject these lines if they were to be uncommented?
If we're not permitted to reject them, then we must accept them, and so you might as well uncomment them permanently. OTOH, if they're supposed to be some sort of "library IFNDR," such that a conforming library might reject them, then it makes sense not to uncomment them (but in that case I'd just delete these lines permanently).

libcxx/test/std/concepts/callable/functions.h
27–28
cjdb updated this revision to Diff 331625.Mar 18 2021, 11:01 AM

rebases because this patch is super old

cjdb marked an inline comment as done.Mar 18 2021, 11:02 AM
cjdb added inline comments.
libcxx/test/std/concepts/callable/functions.h
27–28

No, this is definitely correct as-is.

libcxx/test/std/concepts/callable/functions.h
27–28

There is no conceivable library bug that could cause us to treat int Value = false; and int Value = 0; any differently.
Also, you don't want to return int Value from operator bool; you want to return a bool.

Maybe you meant to write bool Value = false; instead of int Value = false;, is that the issue?

Can we please change the .compile.cpp to just .pass.cpp. There's very little to no cost to actually run the test, and doing so seems safer to me.
It avoids allowing runtime tests to be mistakenly added to these files without anybody noticing.

libcxx/test/std/concepts/callable/strictweakorder.compile.pass.cpp
83

Commented out tests?

cjdb updated this revision to Diff 331642.Mar 18 2021, 11:32 AM
cjdb marked 3 inline comments as done.

fixes out of date thing

cjdb marked an inline comment as done.Mar 18 2021, 11:33 AM
cjdb added inline comments.
libcxx/test/std/concepts/callable/equiv.compile.pass.cpp
25

I'm not interested in arguing over whitespace. Once clang-format handles concepts properly, I'll remove the more offensive // clang-format off.

88

They're INFDR, and I want to document that they're not forgotten. See http://eel.is/c++draft/res.on.requirements.

zoecarver added inline comments.Mar 21 2021, 3:17 PM
libcxx/test/std/concepts/callable/functions.h
27–28

@Quuxplusone why do you want to remove noexcept?

libcxx/test/std/concepts/callable/functions.h
27–28

Well, I'm always on the side of "keep it simple/short." Nothing in C++ cares about whether your conversion-to-bool operator is noexcept, so there's no reason to make it noexcept. (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.)

zoecarver added inline comments.Mar 21 2021, 3:28 PM
libcxx/test/std/concepts/callable/equiv.compile.pass.cpp
88

There's no other place that we'd say, "here are all the ways we can get this to compile with incorrect semantics." Your comment with Relation::Greater doesn't make sense because it's not reflexive, transitive, or symmetric. If we had some way to test that Relation::Greater didn't model equivalence_relation, that would make sense, but, because this is purely semantic, we can't do that.

I don't think it's a bad idea to clarify that these relations haven't been forgotten and have instead been explicitly omitted. But, I think the comment could be more clear/concise about it. I think you should either put a comment at the top (like you have) stating that these relations are explicitly not tested, or update the comments to say NotEquivalenceRelation or !std::equivalence_relation so it's clear that they're semantically not equivalence relations.

zoecarver added inline comments.Mar 21 2021, 3:29 PM
libcxx/test/std/concepts/callable/functions.h
27–28

Fair enough. FWIW I don't feel strongly one way or another about the noexcept.

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

@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

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
314

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
297

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
297

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
297

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 ↗(On Diff #335175)

Should this say "13.0"?

libcxx/include/concepts
314

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
78 ↗(On Diff #335364)

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.