[libcxx-commits] [PATCH] D80899: [libcxx] adds std::compare_three_way

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added a comment.

My 2 cents.



================
Comment at: libcxx/include/functional:151
+    constexpr auto operator()(T&& t, U&& u) const;
+};
+
----------------
Shouldn't you keep `using is_transparent = unspecified;` in synopsis?


================
Comment at: libcxx/include/functional:516
+#if !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)
+#include <compare>
+#endif // __LIBCPP_HAS_NO_SPACESHIP_OPERATOR
----------------
Shouldn't we avoid conditional includes within libc++?


================
Comment at: libcxx/test/std/utilities/function.objects/comparisons/compare_three_way.pass.cpp:13
+
+// less
+
----------------
Nit: less? why not compare_three_way?


================
Comment at: libcxx/test/std/utilities/function.objects/comparisons/compare_three_way.pass.cpp:64
+
+  constexpr std::strong_ordering foo = std::compare_three_way()(36, 36);
+  static_assert(foo == std::strong_ordering::equal, "");
----------------
Sorry for being repetitive in my reviews :). Could you make the run-time and compile-time tests equivalent?
I imagine that this might need to get rid of `reinterpret_cast` from `do_pointer_comparison_test` when making it constexpr.


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

https://reviews.llvm.org/D80899