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

[Christopher Di Bella via Phabricator via libcxx-commits]

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

Thanks for writing this! Some comments, but it should get to LGTM stage pretty soon.



================
Comment at: libcxx/CREDITS.TXT:27-29
-N: Ruben Van Boxem
-E: vanboxem dot ruben at gmail dot com
-D: Initial Windows patches.
----------------
Please defer all of the changes in this file to a dedicated non-functional-change patch.


================
Comment at: libcxx/include/tuple:135-139
 template<class... T, class... U> bool operator<(const tuple<T...>&, const tuple<U...>&);  // constexpr in C++14
 template<class... T, class... U> bool operator!=(const tuple<T...>&, const tuple<U...>&); // constexpr in C++14
 template<class... T, class... U> bool operator>(const tuple<T...>&, const tuple<U...>&);  // constexpr in C++14
 template<class... T, class... U> bool operator<=(const tuple<T...>&, const tuple<U...>&); // constexpr in C++14
 template<class... T, class... U> bool operator>=(const tuple<T...>&, const tuple<U...>&); // constexpr in C++14
----------------



================
Comment at: libcxx/include/tuple:1311
+template <class ..._Tp, class ..._Up, size_t ..._Is>
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+auto
----------------



================
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;
----------------
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.


================
Comment at: libcxx/include/tuple:1321
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+auto
+operator<=>(const tuple<_Tp...>& __x, const tuple<_Up...>& __y)
----------------
As much as I like auto return types, I think it'd be good to match the standard here.


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


================
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; }
----------------
Since this type has exactly one use-case, please define it close to its point-of-use.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:28
+  friend constexpr bool operator==(int, const CustomResult&) noexcept { return true; }
+  friend constexpr bool operator==(const CustomResult&, int) noexcept { return true; }
+  friend constexpr bool operator<(int, const CustomResult&) noexcept { return false; }
----------------
You can delete this line.


================
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; }
+};
----------------
This should return the opposite of the other overload.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250