[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/test/std/utilities/tuple/tuple.tuple/tuple.rel/gt_incompatible.compile.fail.cpp:24
+  // c++20 onwards
+  // expected-error at -2 0-1 {{invalid operands to binary expression}}
+  static_cast<void>(std::tuple<int, long>(1, 2) > std::tuple<int>(1));
----------------
ldionne wrote:
> All those `expected-error`s with `0-N` are not very strong assertions, since the test would pass even if there was no error (because 0 errors is okay).
> 
> The more I read those tests, the more I question their benefit. The Standard says that the program is ill-formed if the tuples don't have the same size, and we usually don't test the ill-formedness of programs (because there are so many programs that are ill-formed, and it's difficult to draw the line of where to stop).
> 
> At the same time, I love test coverage and I think there *is* value in testing that comparing two differently-sized tuples doesn't compile. If it did compile, I'd be worried that something is wrong in our implementation. Perhaps I would suggest the following approach in a single file:
> 
> ```
> #include <tuple>
> 
> void f(std::tuple<int> t1, std::tuple<int, long> t2) {
>   static_cast<void>(t1 == t2); // expected-error@*:* {{}}
>   static_cast<void>(t1 != t2); // expected-error@*:* {{}}
>   static_cast<void>(t1 > t2); // expected-error@*:* {{}}
>   static_cast<void>(t1 >= t2); // expected-error@*:* {{}}
>   static_cast<void>(t1 < t2); // expected-error@*:* {{}}
>   static_cast<void>(t1 <= t2); // expected-error@*:* {{}}
> }
> ```
> 
> IMO this is the most concise way to test that comparing tuples with different sizes doesn't compile. It isn't strongly tied to the libc++ implementation, too, so it can live in `libcxx/test/std`. WDYT?
IMHO, if we can't rewrite these tests to be SFINAE-friendly as in `three_way_incompatible.compile.pass.cpp` (and previous comments already indicate: no, we can't), then we just shouldn't bother with them.

I believe your suggestion's 6 instances of `// expected-error@*:* {{}}` are equivalent to just 1 instance, right? Because if you're saying `@*:*` then it doesn't really matter what line(s) you put the comment on. But I agree that splitting that test into 6 different test files seems like overkill.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250