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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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
----------------
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).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80891