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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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; })
----------------
jloser wrote:
> philnik wrote:
> > Mordante wrote:
> > > 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?
> > You are allowed to specialize `std::common_type` for types that depend on at least one user-defined type. That means that you could theoretically specialize it for the identity and return a different type. (`std::common_type<T>` forwards to `std::common_type<T, T>`) I think you actually want to `type_identity_t<basic_string_view<...>>` instead, because that guarantees that you get `basic_string_view<...>` back.
> `type_identity_t` does work for creating the non-deduced context which means that only one argument would participate in template argument deduction and then the other one would get implicitly converted to the deduced type. Note this is the "sufficient overloads" wording in the standard for `string_view`'s comparison operators.
> 
> However, I'm not convinced //yet// that our implementation is broken for the reason you claim @philnik.  Can you add a test to show it's a problem on top-of-tree with the current comparison/relative operators?  Note that whatever we do for `operator<=>`, we should do the same for the comparison/relative operators (`==`, `!=`, `<`, etc.). Currently, we use `common_type` there and not `type_identity_t`.
> 
> In particular, take a look at the current tests for the sufficient overloads at `libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp` and friends.
[Here](https://godbolt.org/z/da7TnoThE) is a reproducer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130295