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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
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 {
----------------
Quuxplusone wrote:
> `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.
There at least two advantages to it being a lambda:
* no ADL issues
* noexcept is correct by default (you're missing a noexcept specifier on this)


================
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;
----------------
Quuxplusone wrote:
> 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?
> 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`);

In which situation is this possible? If `three_way_comparable` is false, then it's `weak_ordering`. If the concept is true, then it's only allowed to be one of `partial_ordering`, `weak_ordering`, or `strong_ordering`.


================
Comment at: libcxx/include/__utility/pair.h:339
+    if (__result == 0) return __synth_three_way(__x.second, __y.second);
+    else return __result;
+}
----------------
Please replace the if-statement with a conditional operator or match the standard's if-statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721