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

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
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)
----------------
Mordante wrote:
> 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.
I split this apart into https://reviews.llvm.org/D114561


================
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");
----------------
Mordante wrote:
> Can you test whether it's !is_constructible, when the not used as const?
I think that's a bit tricky. For example,

```
static_assert(!std::is_constructible_v<std::string_view, decltype(d)>); // fails oddly enough
```

but
```
std::string_view s = d; // hard error
assert(s == "test");
```

hard errors when trying to construct the `std::string_view` since it invokes the deleted implicit conversion operator:

```
libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:66:20: error: conversion function from 'DeletedStringViewConversionOperator' to 'std::string_view' (aka 'basic_string_view<char>') invokes a deleted function
  std::string_view s = d;
```

Did you have something specifically in mind that you think could work?


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:127
+
+int main(int, char**) {
+  test();
----------------
Mordante wrote:
> 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?
Should be doable - good idea. I'll look into that once I figure out the duplicate symbol issue for windows-dll for `std::string_view::operator!=`. My attempted fix using `__type_identity_t` doesn't work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161