[libcxx-commits] [PATCH] D107721: [libc++][spaceship] Implement std::pair::operator<=>
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sun Sep 12 10:11:28 PDT 2021
Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.
LGTM modulo testing comments, which I think are easy to address.
================
Comment at: libcxx/include/__utility/pair.h:314
template <class _T1, class _T2>
inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
----------------
Before this line, I'd appreciate the addition of a comment line just for ease of comparing against the Standard:
```
// [pairs.spec], specialized algorithms
```
================
Comment at: libcxx/include/utility:107
+ synth-three-way-result<T2> >
+ operator<=>(const pair<T1,T2>&, const pair<T1,T2>&); // C++20
----------------
Nit: I think `// C++20` is indented by 4 too many spaces.
I'd also cuddle the angle-brackets `>>` on line 106.
================
Comment at: libcxx/test/libcxx/library/description/conventions/expos.only.func/synth_three_way.pass.cpp:12
+
+// template<class T, class U> auto __synth_three_way(T __t, U __u);
+
----------------
This isn't particularly close to the actual declaration of synth-three-way. Personally I'd just write
```
// constexpr auto __synth_three_way = ...;
```
================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.spec/three_way_comparison.pass.cpp:47-55
+ // Pairs of int (strongly ordered) and double (partially ordered) should compare with partial ordering.
+ assert((std::make_pair(1, 1.0) <=> std::make_pair(1, 2.0)) < 0);
+ assert((std::make_pair(2, 1.0) <=> std::make_pair(1, 1.0)) > 0);
+ assert((std::make_pair(std::numeric_limits<double>::quiet_NaN(), 1)
+ <=> std::make_pair(std::numeric_limits<double>::quiet_NaN(), 2))
+ == std::partial_ordering::unordered);
+ auto same = std::make_pair(0, 0.0) <=> std::make_pair(0, 0.0);
----------------
How about:
```
using P = std::pair<int, double>;
using nan = std::numeric_limits<double>::quiet_NaN();
ASSERT_SAME_TYPE(decltype(P() <=> P()), std::partial_ordering);
assert((P(1, 1.0) <=> P(1, 2.0)) == std::partial_ordering::less);
assert((P(1, 1.0) <=> P(1, 1.0)) == std::partial_ordering::equivalent);
assert((P(1, -0.0) <=> P(1, 0.0)) == std::partial_ordering::equivalent);
assert((P(1, 2.0) <=> P(1, 1.0)) == std::partial_ordering::greater);
assert((P(1, nan) <=> P(2, nan)) == std::partial_ordering::less); // because 1 < 2
assert((P(2, nan) <=> P(1, nan)) == std::partial_ordering::greater); // because 2 > 1
assert((P(1, nan) <=> P(1, nan)) == std::partial_ordering::unordered);
assert((P(1, nan) <=> P(1, 1.0)) == std::partial_ordering::unordered);
```
And then finally I think we should have a SFINAE test, basically like this (IIUC):
```
template<class T> concept HasEqual = requires(T t) { t == t; };
template<class T> concept HasLess = requires(T t) { t < t; };
template<class T> concept HasSpaceship = requires(T t) { t <=> t; };
struct NoCompare {};
static_assert(HasEqual<std::pair<int, NoCompare>>);
static_assert(!HasLess<std::pair<int, NoCompare>>);
static_assert(!HasSpaceship<std::pair<int, NoCompare>>);
```
This SFINAE behavior is a bit bizarre, but it's Standard-conforming IIUC: `(p == p)` is well-formed, it's just going to hard-error if you actually evaluate it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107721/new/
https://reviews.llvm.org/D107721
More information about the libcxx-commits
mailing list