[libcxx-commits] [PATCH] D113161: [libc++] Implement P1989R2: range constructor for string_view

[Joe Loser via Phabricator via libcxx-commits]

jloser marked an inline comment as done.
jloser added a comment.

In D113161#3109084 <https://reviews.llvm.org/D113161#3109084>, @Quuxplusone wrote:

> Some comments, dumped out-of-line since you're still working on this.
>
> - Using `same_as` for `is_same_v` is probably harmless, but using `convertible_to` in lieu of `is_convertible_v` changes the behavior. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1989r2.pdf says to use `is_convertible_v`. (Personally I would also use `is_same_v` as written, because it's cheaper.)
>
> - You can use the injected-class-name: instead of `d.operator _VSTD::basic_string_view<_CharT, _Traits>();` you can say `d.operator basic_string_view();` (and personally, I would).

Agree regarding type traits vs concepts use. I originally wrote the constructor using type traits and then switched to concepts. Back to type traits we go :)

As for using the injected-class-name, are you sure? What if a user has their range type that is implicitly convertible to `boost::basic_string_view` for example and brings `boost` namespace into scope via a using declarative (same argument for any namespace with the same template `basic_string_view`)? We want this constraint to only look at the conversion operator for `std::basic_string_view` (not just any `basic_string_view`). Will that be OK still if we use the injected-class-name? I think the answer is yes like you said as the injected-class-name here should always refer to `::std::basic_string_view<_CharT, _Traits>`, but wanted to talk through this. Note the standard explicitly says `::​std​::​basic_­string_­view<charT, traits>` FWIW.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161