This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adds concepts std::equality_comparable[_with]
ClosedPublic

Authored by cjdb on Feb 21 2021, 8:12 PM.

Details

Summary

Implements parts of:

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

Depends on D96660

Diff Detail

Event Timeline

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

I like the extensive set of tests you added! I'd like to see the build pass before accepting.

libcxx/include/concepts
258

t -> __t and u -> __u

cjdb updated this revision to Diff 325660.Feb 22 2021, 8:47 PM

fixes CI problem

cjdb updated this revision to Diff 325681.Feb 22 2021, 10:28 PM

Uglifies names

ldionne accepted this revision.Mar 2 2021, 9:05 AM

The patch LGTM, but we should fix the GCC CI before committing.

This revision is now accepted and ready to land.Mar 2 2021, 9:05 AM
cjdb updated this revision to Diff 327580.Mar 2 2021, 1:56 PM

rebases to activate CI

cjdb updated this revision to Diff 327931.Mar 3 2021, 2:50 PM

fixes GCC build errors:

Quuxplusone accepted this revision.Mar 3 2021, 5:35 PM
Quuxplusone added a subscriber: Quuxplusone.

Just some nits on tests. The concept definition looks like a straightforward transliteration of
http://eel.is/c++draft/concept.booleantestable
http://eel.is/c++draft/concept.equalitycomparable
so LGTM.

libcxx/test/std/concepts/comparison/types.h
16

Would anything change if this operator were explicit? In production code I would expect their operator bool to be explicit, because all constructors and conversions should always be explicit (and for bool conversions in particular, the explicit doesn't normally get in the way, because contextual conversion to bool).

51

Utter nit: I don't think most production code would bother to mark operator== as noexcept, so unless some test is relying on this, it might be better to remove the noexcept (for realism and to save characters). Or put it on some and not others (but with a comment explaining that that's on purpose).

friend int operator==(explicit_operators, eq_neq_different_return_types);

is already missing the noexcept, but without the comment explaining that it was on purpose. ;)

86

I'd say deleted_ne. This case is (intentionally) non-parallel to the no_eq case above.

164

Should this == say !=? At a glance, I have no idea what the overload set ends up looking like here. Which if that's the point, it needs a comment. ;)

cjdb added inline comments.Mar 3 2021, 5:52 PM
libcxx/test/std/concepts/comparison/types.h
16

Yes. boolean-testable refines convertible_to<T, bool>[1], which requires T be both implicitly and explicitly convertible to bool. Would you like an explicit_bool test as well?

[1]: http://eel.is/c++draft/concept.booleantestable

86

I intended them to mirror one another, so I'm not sure I understand what you're getting at.

164

Hmm... good point. I had to stop and think about it for a moment myself, so I agree some documentation is necessary (probably a static_assert to prove my intentions and code match). The overload set should look like this:

bool operator==(one_way_ne const&) const&;
bool operator!=(one_way_ne const&) const&; // generated

bool operator==(one_way_ne, explicit_operators);
bool operator!=(one_way_ne, explicit_operators) = delete;
bool operator==(explicit_operators), one_way_ne); // generated
Quuxplusone added inline comments.Mar 3 2021, 6:03 PM
libcxx/test/std/concepts/comparison/types.h
86

no_neq has both an operator== and an operator!=, so when you try to do x != x you get the user-provided operator!= — which happens to be deleted, so x != x is ill-formed.

no_eq has an operator!= and no operator==, so when you try to do x == x... well, okay, I had thought that you'd get a synthesized candidate (if I'm using that term right) formed by negating x != x. I see from Godbolt that that doesn't actually happen. So indeed, x == x also ends up ill-formed.

IOW I was assuming that this asymmetry would materially affect your test results... but it seems that it doesn't really affect them as much as I thought. So nvm.

cjdb updated this revision to Diff 327990.Mar 3 2021, 7:26 PM
cjdb marked 3 inline comments as done.
  • fixes <mutex> problem (hopefully).
  • applies @Quuxplusone's feedback.
  • adds a few extra boolean-testable tests per @Mordante's request in D96477.
libcxx/test/std/concepts/comparison/types.h
51

Maybe I'm in some niche for putting noexcept everywhere possible (I am apparently for top-level const). I've added a comment on line 14.

86

Yeah, only operator== and operator<=> have magical powers 🤷

164

PTAL

Quuxplusone accepted this revision.Mar 4 2021, 1:56 PM

LGTM!
(Obnitpick: I see a mix of auto operator== and bool operator==. Is auto operator== good C++20 style? Anyway, I don't think it ever matters for this particular test.)

cjdb added a comment.Mar 4 2021, 5:17 PM

LGTM!
(Obnitpick: I see a mix of auto operator== and bool operator==. Is auto operator== good C++20 style? Anyway, I don't think it ever matters for this particular test.)

auto operator==(...) = default is ill-formed (it must be bool), but auto operator<=>(...) = default is allowed (the compiler will do magic to work out which of the three blessed types it'll be).

This revision was automatically updated to reflect the committed changes.
Quuxplusone added inline comments.Mar 4 2021, 7:15 PM
libcxx/test/std/concepts/comparison/types.h
129

@cjdb: Here's a remaining auto operator==. You said a defaulted operator== should always be bool — yep, paper standard says https://eel.is/c++draft/class.eq#1 — but I was originally asking about all instances of operator==/operator!= in this file, including non-defaulted and deleted ones.

Looks like all vendors currently accept =delete'd functions with auto return types, and they're probably correct to do so. But I still think stylistically it would be preferable to say bool here.

(An interesting question, whose answer I don't currently know, is whether the difference between putting auto and bool here is detectable by a well-formed program.)