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

[Kent Ross via Phabricator via libcxx-commits]

mumbleskates added inline 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);
 }
----------------
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...


================
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;
----------------
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`).


================
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);
 }
----------------
Furthermore, should this not also check that `(t2 <=> t1 == (0 <=> order))`? (and, depending on consensus above, check the type of `t2 <=> t1`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131372