[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/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");
----------------
Quuxplusone wrote:
> Mordante wrote:
> > jloser wrote:
> > > 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?
> > Interesting, this is the test I expected to work...
> I'm not sure I'm parsing either of your comments correctly, but yeah I would have expected it to be simple to add these lines to this test:
> ```
> static_assert(!std::is_constructible_v<std::string_view, DeletedStringViewConversionOperator>);
> static_assert(std::is_constructible_v<std::string_view, const DeletedStringViewConversionOperator>);
> static_assert(std::is_constructible_v<std::string_view, DeletedConstStringViewConversionOperator>);
> static_assert(!std::is_constructible_v<std::string_view, const DeletedConstStringViewConversionOperator>);
> ```
> @jloser, are you saying that one of those lines gives a hard error? I don't //think// it should.
> 
> Also, naming nit: the words `StringView` aren't pulling their weight and could be omitted: `DeletedConversionOperator`, `DeletedConstConversionOperator`.
Renamed. Both 

```
static_assert(!std::is_constructible_v<std::string_view, DeletedStringViewConversionOperator>);
static_assert(!std::is_constructible_v<std::string_view, const DeletedConstStringViewConversionOperator>);
```

fail currently. I'll need to dig into it.


================
Comment at: libcxx/test/std/strings/string.view/string.view.deduct/range.pass.cpp:43
 int main(int, char**) {
   test();
 
----------------
Quuxplusone wrote:
> Is the lack of `static_assert(test());` here an accidental oversight? I see a lot of `constexpr` above that doesn't make sense otherwise.
It's not an oversight. No `constexpr string` yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161