[libcxx-commits] [PATCH] D107721: [libc++][spaceship] Implement std::pair::operator<=>

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM modulo testing comments, which I think are easy to address.



================
Comment at: libcxx/include/__utility/pair.h:314
 
 template <class _T1, class _T2>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
Before this line, I'd appreciate the addition of a comment line just for ease of comparing against the Standard:
```
// [pairs.spec], specialized algorithms
```


================
Comment at: libcxx/include/utility:107
+                                     synth-three-way-result<T2> >
+    operator<=>(const pair<T1,T2>&, const pair<T1,T2>&);                               // C++20
 
----------------
Nit: I think `// C++20` is indented by 4 too many spaces.
I'd also cuddle the angle-brackets `>>` on line 106.


================
Comment at: libcxx/test/libcxx/library/description/conventions/expos.only.func/synth_three_way.pass.cpp:12
+
+// template<class T, class U> auto __synth_three_way(T __t, U __u);
+
----------------
This isn't particularly close to the actual declaration of synth-three-way. Personally I'd just write
```
// constexpr auto __synth_three_way = ...;
```


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp:47-55
+    // Pairs of int (strongly ordered) and double (partially ordered) should compare with partial ordering.
+    assert((std::make_pair(1, 1.0) <=> std::make_pair(1, 2.0)) < 0);
+    assert((std::make_pair(2, 1.0) <=> std::make_pair(1, 1.0)) > 0);
+    assert((std::make_pair(std::numeric_limits<double>::quiet_NaN(), 1)
+                   <=> std::make_pair(std::numeric_limits<double>::quiet_NaN(), 2))
+                  == std::partial_ordering::unordered);
+    auto same = std::make_pair(0, 0.0) <=> std::make_pair(0, 0.0);
----------------
How about:
```
using P = std::pair<int, double>;
using nan = std::numeric_limits<double>::quiet_NaN();
ASSERT_SAME_TYPE(decltype(P() <=> P()), std::partial_ordering);
assert((P(1, 1.0) <=> P(1, 2.0)) == std::partial_ordering::less);
assert((P(1, 1.0) <=> P(1, 1.0)) == std::partial_ordering::equivalent);
assert((P(1, -0.0) <=> P(1, 0.0)) == std::partial_ordering::equivalent);
assert((P(1, 2.0) <=> P(1, 1.0)) == std::partial_ordering::greater);
assert((P(1, nan) <=> P(2, nan)) == std::partial_ordering::less);  // because 1 < 2
assert((P(2, nan) <=> P(1, nan)) == std::partial_ordering::greater);  // because 2 > 1
assert((P(1, nan) <=> P(1, nan)) == std::partial_ordering::unordered);
assert((P(1, nan) <=> P(1, 1.0)) == std::partial_ordering::unordered);
```

And then finally I think we should have a SFINAE test, basically like this (IIUC):
```
template<class T> concept HasEqual = requires(T t) { t == t; };
template<class T> concept HasLess = requires(T t) { t < t; };
template<class T> concept HasSpaceship = requires(T t) { t <=> t; };

struct NoCompare {};
static_assert(HasEqual<std::pair<int, NoCompare>>);
static_assert(!HasLess<std::pair<int, NoCompare>>);
static_assert(!HasSpaceship<std::pair<int, NoCompare>>);
```
This SFINAE behavior is a bit bizarre, but it's Standard-conforming IIUC: `(p == p)` is well-formed, it's just going to hard-error if you actually evaluate it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721