[libcxx-commits] [PATCH] D131372: [libc++][spaceship] Implement std::variant::operator<=>

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for your comments.



================
Comment at: libcxx/test/support/test_comparisons.h:155
     AssertComparisonsReturnBool<T, U>();
     ASSERT_SAME_TYPE(decltype(std::declval<const T&>() <=> std::declval<const U&>()), Order);
 }
----------------
mumbleskates wrote:
> Likewise here, I feel that this function should also be asserting the same type is returned when the operands are swapped. Unless there's something I'm forgetting and that can't possibly happen due to how operators are resolved in c++20...
Good point, feel free to add it.


================
Comment at: libcxx/test/support/test_comparisons.h:160-162
+    bool equal   = order == Order::equivalent;
+    bool less    = order == Order::less;
+    bool greater = order == Order::greater;
----------------
mumbleskates wrote:
> Here's a question for you all: Should these instead be spelled as comparisons against zero? It seems possibly better not to assume the ordering type has normally named members, right? This seems like it would enable testing against user-typed orderings while assuming nothing about them except the basis of their usage in expressions.
> 
> I also have been wondering if this function should assert the returned type of `t1 <=> t2` (and probably `t2 <=> t1`); other than a recently created test in D130295 this already holds true in the entire test suite, and it seems like an unexpected gotcha that it actually doesn't assert anything about the type. This would especially hold true after the above change, when you could otherwise simply use `int` for the `Order` type (as long as we are not also checking `(t2 <=> t1 == (0 <=> order))`, which would not work with `int`).
The types themselves are normally tested in the static_assert for the return type. So I don't think we need to do it here too. But it seems we didn't do it properly in D130295 so maybe it's useful.

I had a look at the Standard, but is it allowed to return something else as one of the ordering categories?



================
Comment at: libcxx/test/support/test_comparisons.h:164
 
-    return (t1 <=> t2 == order) && testComparisons(t1, t2, equal, less);
+    return (t1 <=> t2 == order) && testComparisonsComplete(t1, t2, equal, less, greater);
 }
----------------
mumbleskates wrote:
> Furthermore, should this not also check that `(t2 <=> t1 == (0 <=> order))`? (and, depending on consensus above, check the type of `t2 <=> t1`)
How do you feel about making a separate patch with the proposed improvements?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131372