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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/tuple:1323
+template <class ..._Tp, class ..._Up>
+requires (sizeof...(_Tp) == sizeof...(_Up))  // return type is ill-formed for non-matching sizes
+_LIBCPP_HIDE_FROM_ABI constexpr
----------------
Comment unnecessary.


================
Comment at: libcxx/include/tuple:1324
+{
+    static_assert (sizeof...(_Tp) == sizeof...(_Up), "Can't compare tuples of different sizes");
+    return __tuple_compare_three_way(__x, __y, index_sequence_for<_Tp...>{});
----------------
mumbleskates wrote:
> cjdb wrote:
> > There isn't a constraints or mandates paragraph mentioned in `operator<=>`, so we can't do this. The note at the bottom states:
> > 
> > > Also, all comparison operator functions are short circuited; they do not perform element accesses beyond what is required to determine the result of the comparison.
> > 
> > This implies that `tuple{0, 1, 2} <=> tuple{0, 2}` is allowed. This will have a knock-on effect for your tests.
> The standard does not require `operator<=>` between tuples of different lengths to exist. The return type as written in the standard, `common_comparison_category_t<synth-three-way-result <TTypes, UTypes>...>`, is ill-formed if the tuples have different sizes.
> 
> I spent some time looking into this and everything that I can find indicates that comparisons between tuples with mismatching sizes are not defined in the standard.
Right, I get it now.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:30
+  friend constexpr bool operator<(int, const CustomResult&) noexcept { return false; }
+  friend constexpr bool operator<(const CustomResult&, int) noexcept { return false; }
+};
----------------
mumbleskates wrote:
> cjdb wrote:
> > This should return the opposite of the other overload.
> It's the equivalent of `lhs > rhs` (not `>=`), and CustomResult is emulating equality here so both of those ought to be `false`.
Please have the type's name reflect that this is emulating equality.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way_incompatible.fail.cpp:25-26
+{
+  static_cast<void>(std::tuple<int>(1) <=> std::tuple<int, long>(1, 2)); // expected-error {{invalid operands to binary expression}}
+  static_cast<void>(std::tuple<int, long>(1, 2) <=> std::tuple<int>(1)); // expected-error {{invalid operands to binary expression}}
+  return 0;
----------------
Please change this to a passing compile-time test and use a requires-expression. Similarly in other failure files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250