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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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; })
----------------
philnik wrote:
> 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.
I think @philnik's right. We should add a test and use `__type_identity_t` -- regardless, `__type_identity_t` makes more sense in that context, IDK why the code was written with `common_type<T>::type` in the first place (does anybody know?).

I would tackle that as a separate patch since this is a fix to the existing code.


================
Comment at: libcxx/test/support/constexpr_char_traits.h:27
+    // The comparison_category is omitted so the class will have weak_ordering
+    // in C++20. This is intentionally.
 
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130295