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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added inline comments.


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

It's aligned perfectly with the other comparison operators above it for me. There are no tabs in this file, and dedenting it by 4 doesn't line it up with anything so I'm not sure what you're seeing.

> I'd also cuddle the angle-brackets `>>` on line 106.

Done.


================
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);
----------------
Quuxplusone wrote:
> 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.
> `assert((P(1, nan) <=> P(1, 1.0)) == std::partial_ordering::unordered);`

This cannot be evaluated constexpr by GCC, and needs its own non-constexpr-only function. As this is annoying and doesn't particularly add any information that we don't already get by comparing `NaN <=> NaN` to get `unordered`, I've been leaving these cases out. (GCC rules that `A @ B` can't be evaluated constexpr if only one of the operands is NaN; there are similar rules for inf.)

> 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.

Would it be better to define `operator==` in `NoCompare` to make this less strange? How about this, which achieves the same end but is less weird:

```
    struct NoRelative {
      constexpr bool operator==(const NoRelative&) const = default;
    };
    static_assert(HasEqual<std::pair<int, NoRelative>>);
    static_assert(!HasLess<std::pair<int, NoRelative>>);
    static_assert(!HasSpaceship<std::pair<int, NoRelative>>);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721