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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for working on this.



================
Comment at: libcxx/include/__compare/synth_three_way.h:1
+// THIS FILE NOT FOR REVIEW (see https://reviews.llvm.org/D107721 )
+//===----------------------------------------------------------------------===//
----------------
You can use the stack feature of Phabricator to make patch series. That makes reviewing the real changes easier.


================
Comment at: libcxx/include/tuple:135
 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
-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, until C++20
+template<class... T, class... U> bool operator!=(const tuple<T...>&, const tuple<U...>&); // constexpr in C++14, until C++20
----------------
For clarity please s/until/removed in/ 


================
Comment at: libcxx/include/tuple:1318
+    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;
----------------
Please use `_VSTD::get`.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:33
+
+int main(int, char**)
+{
----------------
Please change the test to use our common idom to test these `constexpr` functions both runtime and compile time. See `test/libcxx/ranges/range.adaptors/range.copy.wrap/arrow.pass.cpp` for a simple example.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:38
+        typedef std::tuple<> T0;
+        static_assert((T0() <=> T0()) == std::strong_ordering::equal, "");
+        // No member types yields strong ordering (all are equal).
----------------
The empty string in the `static_assert` is not required in C++20, please remove it.


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.rel/three_way.pass.cpp:73
+    }
+    // Mixed types with floating point, which compare partially ordered
+    {
----------------
It would be nice to also mix signed and unsigned types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108250