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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates marked an inline comment as done.
mumbleskates added inline comments.


================
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}}
----------------
Quuxplusone wrote:
> 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).
I can rewrite these as `static_assert`s or something instead of cast-to-void, but it is not possible to SFINAE test these cases because `operator==` (and, prior to c++20, all the other operators) are ruled out as ill-formed by `static_assert`, not by participation. cjdb had similar questions but it turns out none of the tests that are compile.fail can be written as compile.pass SFINAE (in the case of `operator==`, even in c++>=20).


================
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);
+  }
----------------
Quuxplusone wrote:
> 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?
`operator==` does not fallback to be rewritten in terms of `operator<=>`, so yeah tuple's `operator==` is expectedly undefined in this case.

It doesn't really hurt to test it, but I think this would belong in the `eq.pass` test instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250