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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added inline comments.


================
Comment at: libcxx/include/tuple:1315
+    common_comparison_category_t<__synth_three_way_result<_Tp, _Up>...> __result = strong_ordering::equal;
+    static_cast<void>(((__result = _VSTD::__synth_three_way(get<_Is>(__x), get<_Is>(__y)), __result != 0) || ...));
+    return __result;
----------------
cjdb wrote:
> I really like what you're doing here and wish that we could keep it, but I don't think it meshes with the fact we can compare tuples of different lengths.
As far as I am aware, we cannot compare tuples of different lengths.


================
Comment at: libcxx/include/tuple:1321
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+auto
+operator<=>(const tuple<_Tp...>& __x, const tuple<_Up...>& __y)
----------------
cjdb wrote:
> As much as I like auto return types, I think it'd be good to match the standard here.
Yeah I just wrote it this way because it's less wordy. I can change `auto` to the full return type here, but it will be exactly duplicated by the declared type of `__result`, the only thing that is ever returned. Since these are different functions I'll just copy it here, that's fine.


================
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...>{});
----------------
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.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:26
+// A custom three-way result type
+struct CustomResult {
+  friend constexpr bool operator==(int, const CustomResult&) noexcept { return true; }
----------------
cjdb wrote:
> Since this type has exactly one use-case, please define it close to its point-of-use.
I would define it locally but those operators apparently have to be friends, and it's much more compact to define it here (because the friend operators can only be defined non-locally).


================
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; }
+};
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250