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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM with some comments, thanks a lot!



================
Comment at: libcxx/include/string_view:302
+      requires (
+        !is_same_v<remove_cvref_t<_Range>, basic_string_view> &&
+        ranges::contiguous_range<_Range> &&
----------------
jloser wrote:
> 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.
> 
> 
Ahaha I just checked again and I don't know how I could miss that! Thanks for correcting me.


================
Comment at: libcxx/include/string_view:742
 
-template<class _CharT, class _Traits>
+template<class _CharT, class _Traits, int = 1>
 _LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
----------------
Can you add a short comment here saying something like

```
// The dummy default template parameters are used to work around a MSVC issue with mangling, see <link-to-issue> for details.
```


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:81-84
+  NonConstConversionOperator nc;
+  std::string_view sv1  = nc;
+  assert(sv1 == "NonConstConversionOp");
+	static_assert(!std::is_constructible_v<std::string_view, const NonConstConversionOperator&>); // conversion operator is non-const
----------------
Please use scopes to delimit the different test cases:

```
{
  NonConstConversionOperator nc;
  std::string_view sv1  = nc;
  assert(sv1 == "NonConstConversionOp");
  static_assert(!std::is_constructible_v<std::string_view, const NonConstConversionOperator&>); // conversion operator is non-const
}
```

This makes it easy to see at a glance that this is a test case of its own. It also allows you to keep simple identifiers without numbers in them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161