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

[Joe Loser via Phabricator via libcxx-commits]

jloser marked 2 inline comments as done.
jloser added inline comments.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:108
+    operator std::string_view() const noexcept = delete;
+  };
+
----------------
Quuxplusone wrote:
> Please remove the `noexcept`. Also, let's "Don't inherit from `std` types" here. I think all you need to do is
> ```
> struct DeletedStringViewConversionOperator {
>     const char *data_ = "test";
>     constexpr const char *begin() const { return data_; }
>     constexpr const char *end() const { return data_ + 4; }
>     operator std::string_view() const = delete;
> };
> DeletedStringViewConversionOperator d;
> std::string_view sv = d;
> assert(sv == "test");
> std::string_view csv = std::as_const(d);
> assert(csv == "test");
> ```
> Bonus: this should be constexpr-friendly out of the box.
> 
> It might also be worth testing a similar type with a deleted but //non//-const-qualified `operator string_view`. In that case, I'm having trouble visualizing it, but maybe the effect would be that `sv = d` would be noisily ill-formed and `csv = std::as_const(d)` would be well-formed?
> std::string_view csv = std::as_const(d); 
> assert(csv == "test");

That `string_view` construction results in a hard error since we'd be invoking the deleted conversion operator. I've changed the tests to cover both cases without hard erroring:

```
struct DeletedStringViewConversionOperator {
    const char *data_ = "test";
    constexpr const char *begin() const { return data_; }
    constexpr const char *end() const { return data_ + 4; }
    operator std::string_view() = delete;
};

struct DeletedConstStringViewConversionOperator {
    const char *data_ = "test";
    constexpr const char *begin() const { return data_; }
    constexpr const char *end() const { return data_ + 4; }
    operator std::string_view() const = delete;
};

    DeletedStringViewConversionOperator d;
    std::string_view csv = std::as_const(d);
    assert(csv == "test");
    
    DeletedConstStringViewConversionOperator dc;
    std::string_view sv = dc;
    assert(sv == "test");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161