[libcxx-commits] [PATCH] D80891: [libcxx] adds consistent comparison for `basic_string_view`

Sun May 2 10:25:23 PDT 2021

cjdb added inline comments.

================
Comment at: libcxx/include/__string:793-795
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)
+    typedef strong_ordering comparison_category;
+#endif
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > AFAICT, `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR` controls whether libc++ is able to use the `<=>` token. It has nothing to do with whether we define the `strong_ordering` enumeration or anything like that. So this member typedef should be conditioned on `#if _LIBCPP_STD_VER > 17` only. Ditto throughout: anywhere you've got something C++20-related under `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR`, you shouldn't — //unless// it depends on the compiler being able to parse the `<=>` token, in which case you should.
> > Is there a reason `strong_ordering` should be available when `operator<=>` isn't? The two are very tightly coupled.
> > 
> > Also, I believe @EricWF suggested I use it: https://reviews.llvm.org/D80891?id=267623#inline-749046.
> Right now, `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR` is defined if-and-only-if the //compiler// defines `__cpp_impl_three_way_comparison`. It is not (yet) directly related to whether libc++ itself is planning to define `__cpp_lib_three_way_comparison` — but that might just be because libc++ itself hasn't done anything with `__cpp_lib_three_way_comparison` yet. This D80891 is one of the first steps toward implementing `__cpp_lib_three_way_comparison`.
> 
> So, okay, if we want to say that libc++ will implement `__cpp_lib_three_way_comparison` only on compilers that implement `__cpp_impl_three_way_comparison` (which does seem reasonable), then it makes sense to guard //everything// `<=>`-related under `__cpp_impl_three_way_comparison` (which is to say, under `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR`).
> 
> This logic applies even to
> https://en.cppreference.com/w/cpp/utility/compare/compare_strong_order_fallback
> By this logic, libc++ should provide `std::compare_strong_order_fallback(x, y)` for //types// lacking an `operator<=>` function, but should not provide `std::compare_strong_order_fallback(x, y)` on //compilers// lacking support for the `<=>` //token//.
> 
> 
> In that case, please just update this line to
> ```
> #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)
> ```
> (and ditto throughout).
FWIW I think a lot of the `#if _LIBCPP_STD_VER > 17 && ...` will become just `#if _LIBCPP_STD_VER > 17` in a little over a year, given our new support model.

> Right now, `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR` is defined if-and-only-if the //compiler// defines `__cpp_impl_three_way_comparison`. It is not (yet) directly related to whether libc++ itself is planning to define `__cpp_lib_three_way_comparison` — but that might just be because libc++ itself hasn't done anything with `__cpp_lib_three_way_comparison` yet. This D80891 is one of the first steps toward implementing `__cpp_lib_three_way_comparison`.
> 
> So, okay, if we want to say that libc++ will implement `__cpp_lib_three_way_comparison` only on compilers that implement `__cpp_impl_three_way_comparison` (which does seem reasonable), then it makes sense to guard //everything// `<=>`-related under `__cpp_impl_three_way_comparison` (which is to say, under `_LIBCPP_HAS_NO_SPACESHIP_OPERATOR`).
> 
> This logic applies even to
> https://en.cppreference.com/w/cpp/utility/compare/compare_strong_order_fallback
> By this logic, libc++ should provide `std::compare_strong_order_fallback(x, y)` for //types// lacking an `operator<=>` function, but should not provide `std::compare_strong_order_fallback(x, y)` on //compilers// lacking support for the `<=>` //token//.
> 

Looks like we're on the same page here.

(Off-topic: this context is one where I'm perfectly fine with cppreference being used for convenience, because we're not discussing technical details of proposed code.)

> In that case, please just update this line to
> ```
> #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_SPACESHIP_OPERATOR)
> ```
> (and ditto throughout).

That line's the same as what's present (confirmed by browser find); is there a typo?

================
Comment at: libcxx/include/string_view:704
+auto operator<=>(const basic_string_view<_CharT, _Traits> __lhs,
+                 const common_type_t<basic_string_view<_CharT, _Traits>> __rhs) noexcept {
+  const auto __result = __lhs.compare(__rhs) <=> 0;
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Quuxplusone wrote:
> > > Remove `const` on lines 703 and 704. (Pass-by-const-value is usually a typo for pass-by-const-reference; in this case we want pass-by-value alone.)
> > > Remove `common_type_t` on line 704. Or is it doing something? We do have `__identity_t<X>` now, if you need that... but cppreference seems pretty sure that `operator<=>(basic_string_view, basic_string_view)` is the correct signature.
> > > 
> > > I would make this a hidden friend, unless there's a demonstrable reason not to. Saves typing and saves overload-resolution time.
> > > (Pass-by-const-value is usually a typo for pass-by-const-reference; in this case we want pass-by-value alone.)
> > 
> > If it were a proper declaration, I'd agree. It's a definition, and `const`-qualifying the parameter is desirable for read-only objects.
> > 
> > > ... but cppreference seems pretty sure that `operator<=>(basic_string_view, basic_string_view)` is the correct signature.
> > >
> > > I would make this a hidden friend, unless there's a demonstrable reason not to. Saves typing and saves overload-resolution time.
> > 
> > cpprefernece is not the standard. Please do the required readings before submitting your review.
> I see you're working on hidden-friend-ifying some other operators in D101707, so I hope you get around to these ones soon. As you say over there, hidden friends have well-documented advantages for overload resolution.
> 
> The standard actually provides a reference implementation for this `operator<=>`!  So I guess I'd recommend copying it: http://eel.is/c++draft/string.view.comparison#example-1
> The reference implementation uses `type_identity_t<X>` as I suggested; indeed `common_type_t` is not supposed to be involved here.
> I see you're working on hidden-friend-ifying some other operators in D101707, so I hope you get around to these ones soon. As you say over there, hidden friends have well-documented advantages for overload resolution.

I //really// wish we could do this, but [[ http://eel.is/c++draft/string.view.synop | string.view.synop ]] says they're free functions :(
I'll probably put forward some of the other mothership contributions once ranges work becomes embarrassingly parallel, so feel free to call these out if the synopses are more friendly elsewhere.

> The standard actually provides a reference implementation for this `operator<=>`! So I guess I'd recommend copying it: http://eel.is/c++draft/string.view.comparison#example-1
> The reference implementation uses `type_identity_t<X>` as I suggested; indeed `common_type_t` is not supposed to be involved here.

Thanks, I missed that. On further inspection, it looks like C++17 doesn't use `common_type_t` either (it uses an alias for `decay_t` called `__identity`), so you're right on all counts, not just `operator<=>`! I'm happy to replace //all// `common_type_t`s with an `__identity` that's switched on C++17/C++20. WDYT?

Having said that, it baffles me that `f(T, type_identity_t<T>)` isn't another spelling of `f(T, T)`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80891