[libcxx-commits] [PATCH] D103478: [libc++][compare] Implement three_way_comparable[_with] concepts

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.

LGTM % comments at this point.
I'm still unhappy with the testing story — >200 lines of "this kind of member function pointer isn't comparable, that kind of member function pointer isn't comparable..." — but since the //code// is correct, I'd rather land this as-is than continue with review.



================
Comment at: libcxx/include/__compare/three_way_comparable.h:34-35
+  __partially_ordered_with<_Tp, _Tp> &&
+  requires(__make_const_lvalue_ref<_Tp> __a, __make_const_lvalue_ref<_Tp> __b) {
+    { __a <=> __b } -> __compares_as<_Cat>;
+  };
----------------
rarutyun wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > ```
> > > requires(__make_const_lvalue_ref<_Tp> __t) {
> > >     { __t <=> __t } -> __compares_as<_Cat>;
> > > ```
> > > Let's keep it simple (and close to the reference implementation).
> > We shouldn't deviate from what's in the standard. The concept is defined the way it is way for a reason.
> Agree with Christopher
Ah, I see the reference implementation actually does use `a` and `b`; I must have simplified it on my end when I did D107036. Okay then.
(I do disagree that "the concept is defined the way it is for a reason"; it //could// have been done simpler without `b`, and had exactly the same semantics. But that's moot until someone files an editorial issue.)


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:110
+static_assert(
+    !std::three_way_comparable<int (S::*)() const volatile && noexcept>);
+
----------------
remove linebreak plz


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:228
+static_assert(!std::three_way_comparable<SpaceshipNonConstArgument>);
+}
+
----------------
`} // namespace user_defined`


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:230
+
+int main(int, char**) { return 0; }
----------------
Please remove the `main` function. For `.compile.pass.cpp` tests, which aren't intended to be run (just compiled), we've settled on making sure they don't have a `main`.


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp:121
+static_assert(!check_three_way_comparable_with<void, int (S::*)()>());
+}
+
----------------
`} // namespace fundamentals`
(and please fix the linebreaks throughout the above; thanks for taking care of the other file already)


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp:240
+static_assert(!check_three_way_comparable_with<SpaceshipNonConstArgument>());
+}
+
----------------
`} // namespace user_defined`


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp:242
+
+int main(int, char**) { return 0; }
----------------
and remove `main`


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable_with.compile.pass.cpp:49
+static_assert(check_three_way_comparable_with<int, char>());
+static_assert(check_three_way_comparable_with<int, wchar_t>());
+static_assert(check_three_way_comparable_with<int, double>());
----------------
cjdb wrote:
> Quuxplusone wrote:
> > This specific line is failing on AArch64 and Arm. I suspect that those platforms have a 32-bit unsigned `wchar_t`, and so this line would be failing for the same reason that
> > ```
> > static_assert(std::three_way_comparable<int, unsigned int>);
> > ```
> > would fail. Therefore, the correct action is to remove this platform-dependent line (and lines 55 and 59 too). However, consider adding
> > ```
> > static_assert(!std::three_way_comparable<int, unsigned int>);
> > ```
> > as a sanity check; I think you're missing anything along those lines at the moment.
> Interesting. I'd prefer to section this test off using an include guard, if it's possible.
Nah, just delete the bogus line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103478/new/

https://reviews.llvm.org/D103478