[libcxx-commits] [PATCH] D107721: Implement std::pair::operator<=>

Mon Aug 9 08:01:18 PDT 2021

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

Thanks for working on this! I hope to get D107584 <https://reviews.llvm.org/D107584> landed later today or tomorrow so that you can rebase on it.

================
Comment at: libcxx/include/__compare/__synthesized.h:9-10
+
+#ifndef _LIBCPP___COMPARE___SYNTHESIZED_H
+#define _LIBCPP___COMPARE___SYNTHESIZED_H
+
----------------
This header should be named `__compare/synth_three_way.h` (name after the component, and drop the leading underscores on the filename).

================
Comment at: libcxx/include/__compare/__synthesized.h:24
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)
+
+template <class T, class U>
----------------
Add a comment: `// [expos.only.func]`

================
Comment at: libcxx/include/__compare/__synthesized.h:26
+template <class T, class U>
+constexpr auto __synth_three_way(const T& __t, const U& __u)
+    requires requires {
----------------
`T, U` => `_Tp, _Up`
Also, I'd like to see some discussion (here, and then in the commit message) about why `__synth_three_way` does //not// need to be a lambda object the way it's shown in the Standard. I suspect that since it's used for partly-program-defined types like `std::pair<int, Holder<Incomplete>*>`, the reason may have to do with suppressing ADL... although, no, that doesn't make sense, because we're doing ADL on `__t < __u` on lines 28 and 35 already.

================
Comment at: libcxx/include/__compare/__synthesized.h:42
+template <class T, class U = T>
+using __synth_three_way_result = decltype(__synth_three_way(declval<T&>(), declval<U&>()));
+
----------------
`_VSTD::__synth_three_way` (unless you change it back to a lambda)
also `T, U` => `_Tp, _Up`

================
Comment at: libcxx/include/__utility/pair.h:13-14
 #include <__config>
+#include <__compare/__synthesized.h>
+#include <__compare/common_comparison_category.h>
 #include <__functional/unwrap_ref.h>
----------------
(Once the underscores are removed, these will need to be re-alphabetized.)
Also, `__com < __con`.

================
Comment at: libcxx/include/__utility/pair.h:337-338
+{
+    auto __result = __synth_three_way(__x.first, __y.first);
+    if (__result == 0) return __synth_three_way(__x.second, __y.second);
+    else return __result;
----------------
ADL-proof: `_VSTD::__synth_three_way`, if `__synth_three_way` doesn't become a lambda.

Also, really evil user code could probably detect that you're calling `__result == 0` instead of `__result != 0` (since `__synth_three_way_result<_T1>` could be `Widget`); how about just using the exact implementation from the Standard, two-part `if` and all?

Before addressing this comment, please make the change I suggested in `comparison.pass.cpp`, and re-run your tests. Does my third `NaN` test case actually blow up this implementation?

================
Comment at: libcxx/test/libcxx/compare/synthesized.pass.cpp:11
+
+// template<class T, class U> auto __synth_three_way(T __t, U __u);
+
----------------
Rename the file to `synth_three_way.pass.cpp`. Also I'd recommend putting it under something like
`libcxx/test/libcxx/library/description/conventions/expos.only.func/synth_three_way.pass.cpp`.
(We'd put it next to the tests for `__decay_copy` if we had any, but we don't.)

================
Comment at: libcxx/test/libcxx/compare/synthesized.pass.cpp:21
+int main(int, char**) {
+#if TEST_STD_VER >= 20
+  static_assert(std::__synth_three_way(1, 2) == std::strong_ordering::less);
----------------
This `#if` is true by definition, since the test is unsupported in earlier versions.

================
Comment at: libcxx/test/libcxx/compare/synthesized.pass.cpp:34
+  static_assert(!(NoSpaceship{1} < NoSpaceship{1}), "");
+  static_assert(std::__synth_three_way(NoSpaceship{1}, NoSpaceship{2}) == std::weak_ordering::less);
+  static_assert(std::is_same_v<std::weak_ordering, std::__synth_three_way_result<NoSpaceship, NoSpaceship> >, "");
----------------
Add:
```
static_assert(std::__synth_three_way(NoSpaceship{1}, NoSpaceship{1}) == std::weak_ordering::equivalent);
static_assert(std::__synth_three_way(NoSpaceship{2}, NoSpaceship{1}) == std::weak_ordering::greater);
ASSERT_SAME_TYPE(std::__synth_three_way_result<NoSpaceship, NoSpaceship>, std::weak_ordering);
```
Similarly for the other cases tested below.
Finally, unless someone proves that this is IFNDR, please also test
```
struct CustomSpaceship {
    struct Result { operator std::weak_ordering() const { return std::weak_ordering::equivalent; } };
    Result operator<=>(const CustomSpaceship&) const { return Result(); }
};
ASSERT_SAME_TYPE(decltype(std::__synth_three_way(CustomSpaceship{}, CustomSpaceship{})), CustomSpaceship::Result);
ASSERT_SAME_TYPE(std::__synth_three_way_result<CustomSpaceship, CustomSpaceship>, CustomSpaceship::Result);
```
and make sure you have a test along these lines for `std::pair` as well.

(You might find that `CustomSpaceship` doesn't synth-three-way as written, because it lacks an `operator==`. If so, well, I recommend adding a test for that too.)

================
Comment at: libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/comparison.pass.cpp:41
+  {
+    // Pairs of int and a type with partial_ordering three way comparison should compare with partial ordering.
+    struct WithPartialSpaceship {
----------------
Could you use `double` as your type with partial ordering?
Let's also add a test where the `unordered` result is expected, e.g.
```
std::make_pair(1, 1.0) <=> std::make_pair(2, std::numeric_limits<double>::quiet_NaN())  // expect ::less
std::make_pair(1, 1.0) <=> std::make_pair(1, std::numeric_limits<double>::quiet_NaN())  // expect ::unordered
std::make_pair(1.0, 1) <=> std::make_pair(std::numeric_limits<double>::quiet_NaN(), 2)  // expect ::unordered
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721