[libcxx-commits] [PATCH] D130295: [libc++] Uses operator<=> in string_view

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for the comments! I'll have a look at them.



================
Comment at: libcxx/include/string_view:796
+operator<=>(basic_string_view<_CharT, _Traits> __lhs, basic_string_view<_CharT, _Traits> __rhs) noexcept {
+    if constexpr (requires { typename _Traits::comparison_category; })
+        return static_cast<typename _Traits::comparison_category >(__lhs.compare(__rhs) <=> 0);
----------------
jloser wrote:
> We currently repeat this pattern of code three times, which is about when I'd start to look to refactor it into a common detail function -- what do you think?
> 
> This will eventually (soon) be needed for `operator<=>` for `std::string` I believe, right?
> 
> Either way, from a QoI perspective, what do you think about checking the type of `comparison_category` to make sure it is one of `partial_ordering`, `weak_ordering`, or `strong_ordering` and not some bogus type, like `void` for example?  This would be a bug from the user's perspective and likely IFNDR from the standards perspective, but it could be an easy QoI improvement I think.
> We currently repeat this pattern of code three times, which is about when I'd start to look to refactor it into a common detail function -- what do you think?

I feel these functions are small enough to keep the duplicates.

> This will eventually (soon) be needed for `operator<=>` for `std::string` I believe, right?

No `std::string` will call `std::string_view`'s `operator<=>`. I already have a patch for that.
So there will be no additional duplicates.

> Either way, from a QoI perspective, what do you think about checking the type of `comparison_category` to make sure it is one of `partial_ordering`, `weak_ordering`, or `strong_ordering` and not some bogus type, like `void` for example?  This would be a bug from the user's perspective and likely IFNDR from the standards perspective, but it could be an easy QoI improvement I think.

Good point! Interestingly P1614 "The Mothership has Landed" has no restrictions on `R`, that's why I didn't add it.
However per [string.view.comparison]/4
```
Mandates: R denotes a comparison category type ([cmp.categories]).
```

That was changed by https://wg21.link/lwg3432

(Note that we should be careful with adding non-Standard restrictions. The original wording allowed an `int` by adding a restriction libc++ would become non-conforming. Instead when we want to add restrictions we should file an LWG issue to make it mandated.)





================
Comment at: libcxx/include/string_view:802
+
+template <class _CharT, class _Traits, int = 1>
+_LIBCPP_HIDE_FROM_ABI constexpr auto operator<=>(
----------------
jloser wrote:
> We may want to copy the comment from above to indicate the `int = 1` and `int = 2` dummy non-type template parameters are to work around an MSVC mangling issue as noted above in the `operator==`. It says it applies to the other sufficient overloads below for comparison ops, but I wouldn't be opposed to a comment here for spaceship too. Thoughts?
I didn't do it since the 5 other operators also didn't have it. But I was a bit tempted to do it, since I was a bit surprised myself.


================
Comment at: libcxx/include/string_view:805
+    basic_string_view<_CharT, _Traits> __lhs,
+    typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) noexcept {
+    if constexpr (requires { typename _Traits::comparison_category; })
----------------
philnik wrote:
> jloser wrote:
> > Nit: can use `common_type_t` instead - here and elsewhere.
> I think this should actually be `type_identity_t`. You could theoretically have something like
> ```
> struct CharT {
>   // all the stuff to make it char-like
> };
> 
> struct CharTT {
>   template <class Traits>
>   CharTT(std::basic_string_view<CharT, Traits>);
>  };
> 
> template <class Traits>
> struct std::common_type<std::basic_string_view<CharT, Traits>, std::basic_string_view<CharT, Traits>> {
>   using type = CharTT;
> };
> ```
> right? That would result in a hard error.
> 
Good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130295