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

[Joe Loser via Phabricator via libcxx-commits]

jloser 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; })
----------------
ldionne wrote:
> 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.
Nice test!  It does look to be a bug that we should fix on top-of-tree in the sufficient overload cases.  Good find.

Re your question, @ldionne, I don't know.  When I added the range constructor for `std::string_view` late last year or so, I recall having to reason about the sufficient overloads due to the mangling issue.  At the time, I justified to myself that `common_type_t` was OK as it was currently written (it's been like this for a while), but I didn't consider the fact that users could specialize `common_type`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130295