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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates 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:
> 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.
> 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.

Okay, I'm not 100% sure what the mechanical corner cases are here. However, since it is a "fake" function that I am only defining a common implementation of here so that it can be re-used in {tuple, list, array, vector, set, map, deque, ...}, I feel that the closer we can get to direct usage of the exact definition we have written here the better. Any observable way in which its lookup is a real process that can be observed is a negative. I felt that perhaps regular functions provide a more familiar way to do this, but I am less sure now and at this pass am more confident than I was the first time that templated lambdas are definitely available.

In the first version I have a suspicion that it is guaranteed to work despite ADL shenanigans because it first speaks of `__synth_three_way_result`, which refers to `__synth_three_way` in its same block. For now I'll //both// prepend `_VSTD::` in pair.h //and// revert it to a lambda which I feel more confident about.


================
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 {
----------------
mumbleskates wrote:
> Quuxplusone wrote:
> > 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.
> > 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.
> 
> Okay, I'm not 100% sure what the mechanical corner cases are here. However, since it is a "fake" function that I am only defining a common implementation of here so that it can be re-used in {tuple, list, array, vector, set, map, deque, ...}, I feel that the closer we can get to direct usage of the exact definition we have written here the better. Any observable way in which its lookup is a real process that can be observed is a negative. I felt that perhaps regular functions provide a more familiar way to do this, but I am less sure now and at this pass am more confident than I was the first time that templated lambdas are definitely available.
> 
> In the first version I have a suspicion that it is guaranteed to work despite ADL shenanigans because it first speaks of `__synth_three_way_result`, which refers to `__synth_three_way` in its same block. For now I'll //both// prepend `_VSTD::` in pair.h //and// revert it to a lambda which I feel more confident about.
> ...
> 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.

Yes, I'm unable to locate any place where `synth-three-way` needs to be noexcept; as far as I'm aware, every standard function that uses this mechanism is `noexcept(false)`, and the expository definition of synth-three-way is also `noexcept(false)`.


================
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:
> 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//.
> 
It's true that the NaN tests do not cause the old code to fail, but I have rewritten it to match the standard anyway as requested.


================
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);
----------------
Quuxplusone wrote:
> This `#if` is true by definition, since the test is unsupported in earlier versions.
Are the UNSUPPORTED comment annotations programmatically processed? It's not completely clear to me if the comment has effect or is conventional. I'll delete these guards assuming you mean that it has effect.


================
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> >, "");
----------------
Quuxplusone wrote:
> 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.)
Having a cast operator to `std::weak_ordering` is totally insufficient for an ordering; you also have to totally satisfy that comparison against the comparison-unspecified-zero argument type is boolean-testable for all comparison operators, otherwise it fails to satisfy the concept.

Not only that, the concept requires that the above is satisfied, and that the result type of the three way comparison is understood by `std::common_comparison_category`, which //only// knows about `std::partial_ordering`, `std::weak_ordering`, and `std::strong_ordering`. The most rigorous attempt I have made to produce a new comparison category either fails to be recognized due to the above and falls back to rewritten comparisons using the custom three way return type (which works! the type is, so to speak, on the council but not granted the rank of master) or is IFNDR.

I've added a test for types that define a valid `operator<=>` but are lacking `operator==`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107721