[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 inline comments.


================
Comment at: libcxx/include/string_view:302
+      requires (
+        !is_same_v<remove_cvref_t<_Range>, basic_string_view> &&
+        ranges::contiguous_range<_Range> &&
----------------
ldionne wrote:
> I'm surprised this isn't written in the spec. I assume this is necessary to break ambiguities in case you try to construct a `string_view` from a `string_view const&&` or a `string_view&`? If you construct from a `string_view const&`/`string_view&&`, the copy/move constructor respectively should be preferred, but not for their `&` and `const&&` counterparts. Is my understanding correct?
> 
> Also, do we have a test that breaks if you remove that constraint?
It is written in the Standard as the first constraint:



> remove_­cvref_­t<R> is not the same type as basic_­string_­view

A few tests in the `string_view` suite fail when this constraint is removed. The constraint exists to avoid ambiguities as you stated as I understand it.




================
Comment at: libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp:24
 constexpr void test() {
-  auto val = MAKE_STRING_VIEW(CharT, "test");
-  auto sv = std::basic_string_view(val.begin(), Sentinel(val.end()));
+  auto val = MAKE_STRING(CharT, "test");
+  auto sv = std::basic_string_view(val);
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Can we add a test that exercises the deduction guide from a range that is *not* a `std::string`?
> Something like this could do the trick:
> ```
> #include "test_iterators.h"
> struct Widget {
>     char16_t *data_ = u16"foo";
>     contiguous_iterator<const char16_t*> begin() const { return data_; }
>     contiguous_iterator<const char16_t*> end() const { return data_ + 3; }
> };
> basic_string_view bsv = Widget();
> ASSERT_SAME_TYPE(decltype(bsv), std::basic_string_view<char16_t>);
> ```
@Quuxplusone that LMGTM. I adopted that modulo `s/u16/u` since we the literals are weird (`u8`, but `u`, and `U` for u16 and u32 literals :)).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161