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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/tuple:1328
+{
+    return __tuple_compare_three_way(__x, __y, index_sequence_for<_Tp...>{});
+}
----------------
ADL-proof: `_VSTD::__tuple_compare_three_way`. Admittedly I //think// ADL-proofing doesn't matter so much inside an operator that's already meant to be called via ADL. The only way I can see to trigger this ADL-bomb is something silly and probably-not-conforming like
```
struct Incomplete;
template<class T> struct Holder { T t; };
Holder<Incomplete> *a[2];
std::tuple<Holder<Incomplete>**> t0 = {a+0};
std::tuple<Holder<Incomplete>**> t1 = {a+1};
assert(std::operator<=>(t0, t1) < 0);  // silly, possibly non-conforming
```


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/eq_incompatible.compile.fail.cpp:21-24
+static_cast<void>(std::tuple<int>(1) == std::tuple<int, long>(1, 2)); // expected-note {{requested here}}
+// expected-error-re at tuple:* {{static_assert failed{{.*}} "Can't compare tuples of different sizes"}}
+
+static_cast<void>(std::tuple<int, long>(1, 2) == std::tuple<int>(1)); // expected-note {{requested here}}
----------------
These error messages are weird. It should be complaining about how you can't write `static_cast<void>(x);` at file scope.

I recommend rewriting this test as a compile-only SFINAE test, like [UNTESTED CODE]
```
template<class T, class U>
constexpr bool are_comparable(long, T, U) { return false; }

template<class T, class U>
constexpr auto are_comparable(int, T t, U u)
    -> decltype(t < u)
{ return true; }

static_assert(are_comparable(1, std::tuple<int>(1), std::tuple<int>(2)));
static_assert(are_comparable(1, std::tuple<int>(1), std::tuple<long>(2)));
static_assert(!are_comparable(1, std::tuple<int>(1), std::tuple<int, int>(2)));
static_assert(!are_comparable(1, std::tuple<int, int>(1), std::tuple<int>(2)));
```
I see you did something like this in `three_way_incompatible.compile.pass.cpp` already (but of course there you get to use C++20 whereas here you need C++11).


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:107
+    typedef std::tuple<double> T2;
+    // Comparisons with NaN and non-NaN are non-constexpr in GCC, so both sides must be NaN
+    assert((T1(std::numeric_limits<float>::quiet_NaN()) <=> T2(std::numeric_limits<double>::quiet_NaN()))
----------------
Throughout, please use `constexpr double nan = std::numeric_limits<double>::quiet_NaN();` for readability.
This will eliminate many line-breaks.
In the tests where you use both float and double nan, I think it's fine to just have the one variable, because `(float)nan` is still going to be a float-typed NaN value.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:172-173
+    typedef std::tuple<int, unsigned long, SpaceshipNoEquals> T2;
+    // Spaceship operator with no == operator falls back on the < operator and weak ordering.
+    ASSERT_SAME_TYPE(decltype(T1() <=> T2()), std::weak_ordering);
+  }
----------------
This is somewhat counterintuitive, right? What is the behavior of `T1() == T2()` in this case — does it call tuple's `operator==`, or does it not exist? Should we test that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250