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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM, some comments but as discussed these should be done in separate patches.



================
Comment at: libcxx/include/variant:747
   public:                                                                      \
-    inline _LIBCPP_INLINE_VISIBILITY                                           \
+    _LIBCPP_HIDE_FROM_ABI                                           \
     explicit constexpr __union(__valueless_t) noexcept : __dummy{} {}          \
----------------
mumbleskates wrote:
> Mordante wrote:
> > please fix the formatting, can you also fix line 742?
> i got these a couple updates ago :)
When I wrote the comment it wasn't done, I guess it wasn't marked as done before ;-)


================
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:
> Mordante wrote:
> > 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?
> > 
> i think it makes sense to put it in here, since it seems like a gotcha that the returned order was actually different than the one you provided in the test. it seems trivially true that you should always provide the exact type that you expect to see whenever you call `testOrder`, and even if we assert the type there we still don't obsolete `AssertOrderReturn` since we aren't asserting anything about the basic operators returning `bool`.
> 
> as for non-standard ordering types:
> 
> it's an allowable thing to do because the usages of ordering-typed values are written in terms of `order @ 0`, as are the synthesized operations (for example, when a type only defines `<=>` and maybe `==`). i know that in most places in the standard library, a lot of the things that are doing `<=>` need to return a common comparison category; user ordering types will never mesh with those so they are eliminated by SFINAE, but user orderings *do* work for synthesis of basic comparison operators, and indeed `synth-three-way`.
> 
> if there is a standard function that returns a templated type's `operator<=>` result without gatekeeping on `three_way_comparable_with`, we might return a user ordering there (`compare_three_way_result_t` will resolve user ordering types). i have no idea whether any such cases exist. in the case of `basic_string_view` as one example the standard seems to explicitly state that non-standard ordering types are ill-formed.
> 
> whatever the case for the code under test, it still seems like we can broaden the use-cases for this function by spelling things differently. just because we might not have a use for it in testing the behavior of actual standard functions doesn't mean we can't use it, like sanity checking the behavior of a type that returns custom orderings for testing purposes.
Then let's add it as you suggested. However in that patch it would be good to refer to the Standard wording where this is allowed. This seems quite odd and confusing. (Just like equal and equivalent being the same and path having an equal and equivalent which isn't the same.) 


================
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:
> Mordante wrote:
> > 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?
> yeah at this point i'm planning on doing this :) i can perhaps also try to tackle improving `support/make_string.h` to use char_traits correctly in the `basic_string_view` tests if you don't get there first.
That would be great! I happily leave it to you; I have enough other things I want to work one ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131372