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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/compare:494
+  __partially_ordered_with<_Tp, _Tp> &&
+  requires(__make_const_lvalue_ref<_Tp>& a, __make_const_lvalue_ref<_Tp>& b) {
+    { a <=> b } -> __compares_as<_Cat>;
----------------
Mordante wrote:
> Please use __ugly_names: `a` -> `__a`, `b` -> `__b`.
`__make_const_lvalue_ref` should take care of making this a reference type (i.e., you can remove the "&"). 


================
Comment at: libcxx/include/compare:507
+  __partially_ordered_with<_Tp, _Up> &&
+  requires(__make_const_lvalue_ref<_Tp>& t, __make_const_lvalue_ref<_Up>& u) {
+    { t <=> u } -> __compares_as<_Cat>;
----------------
Mordante wrote:
> Also __ugly_names here.
Same comment as above. 


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:77
+struct S {};
+static_assert(!std::three_way_comparable<int S::*>);
+static_assert(!std::three_way_comparable<int (S::*)()>);
----------------
Do we really need to assert that all these types are not three way comparable? That seems a bit excessive to me (it would be one thing if they were three way comparable, but they're not-- if we use this philosophy for testing, we can have tests of infinite size).


================
Comment at: libcxx/test/std/language.support/cmp/cmp.concept/three_way_comparable.compile.pass.cpp:110
+struct S {
+    auto operator<=>(const S&) const = default;
+};
----------------
Can we get a type where this operator is only available on rvalue references? I //think// that would currently pass, but should not. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103478