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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added a comment.

Also revised the synth_three_way.pass test with similar improvements.



================
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:
> mumbleskates wrote:
> > 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>>);
> > ```
> > 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.)
> 
> Well, that's weird of GCC. I'm still weakly in favor of including these test cases — hiding the GCC-failing ones under `#ifdef __clang__` or `if (!std::is_constant_evaluated())` or `XFAIL: gcc` or whatever — but I don't think it's blocking.
> I //do// ask that you adopt my spelling of `nan` in the tests, even if you delete some of the nan-related lines. The current PR's lines 58-63 are pretty hard to read. (Also, `)<=>` should be `) <=>`; but that'll be more obvious once the `numeric_limits` cruft is reduced.)
> 
> I've just realized that I missed an important test case:
> ```
> assert((P(nan, 1) <=> P(nan, 2)) == std::partial_ordering::unordered);
> ```
> This shows that when the `.first` comparison is unordered, we do //not// go on to tiebreak using the `.second`.
> 
> > How about this, which achieves the same end but is less weird
> 
> Sounds good to me. Just, you don't need a definition of the function in order to test the concepts. A declaration will suffice:
> ```
> struct NoRelative {
>   bool operator==(const NoRelative&) const;
> };
> static_assert(HasEqual<std::pair<int, NoRelative>>);
> static_assert(!HasLess<std::pair<int, NoRelative>>);
> static_assert(!HasSpaceship<std::pair<int, NoRelative>>);
> ```
> Perhaps even add `>`:
> ```
> struct NoRelative {
>   bool operator==(const NoRelative&) const;
>   bool operator>(const NoRelative&) const;
> };
> static_assert(HasEqual<std::pair<int, NoRelative>>);
> static_assert(!HasLess<std::pair<int, NoRelative>>);
> static_assert(!HasSpaceship<std::pair<int, NoRelative>>);
> ```
> I //do// ask that you adopt my spelling of `nan` in the tests, even if you delete some of the nan-related lines.

Your spelling with `using` does not work at all because `quiet_NaN` is a function, not a type; it seems it works just fine with `constexpr double nan = ...;` though.

> Perhaps even add `>`

I added a second test for that one as well, which seems fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721