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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks a lot for working on this. Also, thanks for your patience with the review and for providing such a nice test coverage. I have a few comments but the patch otherwise looks good to me, so let's finish this up and ship it!



================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/eq_incompatible.compile.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Please make those `.compile.fail.cpp` tests be `.verify.cpp` instead, since you are checking for diagnostics. Applies everywhere.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/eq_incompatible.compile.fail.cpp:26-28
+  // expected-error-re at tuple:* {{static_assert failed{{.*}} "Can't compare tuples of different sizes"}}
+  // expected-error-re at __tuple:* {{static_assert failed{{.*}} "tuple_element index out of range"}}
+  // expected-error at tuple:* {{no viable conversion from}}
----------------
Just make those `expected-error-re@*:*`. Using the filenames here is brittle as it breaks when we move stuff around. Applies here and elsewhere.


================
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));
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250