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

[Christopher Di Bella via Phabricator via libcxx-commits]

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:
> 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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80891