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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/string_view:803-810
+_LIBCPP_HIDE_FROM_ABI constexpr auto operator<=>(
+    basic_string_view<_CharT, _Traits> __lhs,
+    typename common_type<basic_string_view<_CharT, _Traits> >::type __rhs) noexcept {
+    if constexpr (requires { typename _Traits::comparison_category; })
+        return static_cast<typename _Traits::comparison_category >(__lhs.compare(__rhs) <=> 0);
+    else
+        return static_cast<weak_ordering>(__lhs.compare(__rhs) <=> 0);
----------------
philnik wrote:
> What breaks if you drop the other two overloads? The compiler should automatically generate the `operator<=>(common_type_t<basic_string_view>, basic_string_view)` IIUC. Or is that just for `operator==`?
One of the two can be removed, when I removed the seconds several tests started to fail.
I didn't do a deep investigation.


================
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; })
----------------
Mordante wrote:
> 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.
I'm not entirely clear what you mean, can you elaborate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130295