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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates marked 4 inline comments as done.
mumbleskates 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));
----------------
Quuxplusone wrote:
> 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.
Okay, I've tried consolidating these tests into a single compilation failure verify test for all six small operators as ldionne showed, and retained the SFINAE compile.pass test for three-way comparison.

This raises a new problem: when you test both `==` and `!=`, they hit the same static_assert and are consumed by the same error expectation, so only 5 errors are emitted for the 6 operators.

I agree with the idea that these should have less specific error expectations, but I am recalling the reasons I originally split these into their own files. I don't know enough about llvm-lit to know the best way to make this work without doing that, or if I should indeed just give up and make it one file per operator again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250