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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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 {
----------------
cjdb wrote:
> 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)
> noexcept is correct by default
If you mean "lambdas are noexcept(auto) by default," no, that's not true. They're constexpr by default, but not noexcept. If the Standard requires noexcept(auto) / expression-equivalency here, then we need to write code to implement it.
However, https://eel.is/c++draft/expos.only.func gives a reference implementation which is `noexcept(false)`. Therefore, at least arguably (and maybe more than arguably), it would be //detectably non-conforming// to add `noexcept(~~~)` here.


================
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;
----------------
cjdb wrote:
> 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`.
> Does my third NaN test case actually blow up this implementation?
No, because `__result != 0` is equivalent to `!(__result == 0)`, even in cases like that where `!(__result < 0 || __result > 0)`.

> In which situation is [`Widget __result`] possible?
Ah, right, it's not. OK.

Still, +1 to @cjdb's comment below: please match the Standard's reference implementation here //anyway//.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721