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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 6 inline comments as done.
Mordante 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; })
----------------
Mordante wrote:
> jloser wrote:
> > Mordante wrote:
> > > jloser wrote:
> > > > 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`.
> > > Agreed I'll look at this issue in a separate patch.
> > @Mordante to clarify, the plan is to stick with `common_type_t` for both the existing comparison operators and the new `operator<=>` and the bug of not using `type_identity_t` (`__type_identity_t` I guess since we backport `string_view` to older standard modes) will be fixed in both in a follow-up? If so, I'm OK with that.
> Yes I prefer to do both in the same commit.
D131322 switches `commont_type_t` to `type_identity_t` for both the spaceship operator as the existing operators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130295