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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Mainly looks good, a few small points.



================
Comment at: libcxx/include/string_view:291
     template <contiguous_iterator _It, sized_sentinel_for<_It> _End>
-      requires (same_as<iter_value_t<_It>, _CharT> && !convertible_to<_End, size_type>)
+      requires (is_same_v<iter_value_t<_It>, _CharT> && !is_convertible_v<_End, size_type>)
     constexpr _LIBCPP_HIDE_FROM_ABI basic_string_view(_It __begin, _End __end)
----------------
jloser wrote:
> Quuxplusone wrote:
> > :+1:
> > I idly wonder if it's possible to make a (contrived) test case that regression-tests this. There's https://stackoverflow.com/a/62644127/1424877 but it works only for conversions to user-defined class types, not to `size_t`.
> Right, it would be nice to test this. I'll think on it some the next few days. It may be addressed in a separate patch to decouple it from this one if I find a way.
I think it would be good to commit this separately from this patch.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:60
+  DeletedStringViewConversionOperator d;
+  std::string_view csv = std::as_const(d);
+  assert(csv == "test");
----------------
Can you test whether it's !is_constructible, when the not used as const?


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:127
+
+int main(int, char**) {
+  test();
----------------
The paper has a Thows clause `Throws:Any exception thrown byranges::data(r)andranges::size(r).`
Can you add some tests to verify that behaviour?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161