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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

Some comments, dumped out-of-line since you're still working on this.

- Using `same_as` for `is_same_v` is probably harmless, but using `convertible_to` in lieu of `is_convertible_v` changes the behavior. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1989r2.pdf says to use `is_convertible_v`. (Personally I would also use `is_same_v` as written, because it's cheaper.)

- You can use the injected-class-name: instead of `d.operator _VSTD::basic_string_view<_CharT, _Traits>();` you can say `d.operator basic_string_view();` (and personally, I would).



================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:75-77
+// TODO: these don't pass yet
+//static_assert(!std::is_constructible_v<std::string_view, WithCharConversionOperator<char>>); // has conversion operator to const CharT*
+//static_assert( std::is_constructible_v<std::string_view, WithCharConversionOperator<char16_t>>); // has conversion operator to different CharT
----------------
I believe these are both as-expected.
`std::string_view(WCCO<char>{})` is well-formed because `WCCO<char>` can be implicitly converted to `const char*`, and then we can certainly use that `const char*` as the argument to `string_view`'s ctor.
`std::string_view(WCCO<char16_t>{})` is ill-formed because it doesn't have anything to do with `char`, so how could we possibly make a `string_view` over it.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:99
+};
+// TODO: why doesn't this pass with inaccessible traits_type?
+//static_assert(!std::is_constructible_v<std::string_view, InaccessibleTraitsType>); // invalid (not accessible) traits_type type
----------------
Well, if `traits_type` is inaccessible from the POV of `string_view`, then obviously `string_view` can't see it. So `string_view` believes that no `traits_type` exists, which means the ctor is not disabled. I believe this is expected.
Aside: It is not possible to befriend concepts, but it //is// possible to befriend the first-user-of-a-concept, and GCC+Clang behave differently from MSVC in that case: https://godbolt.org/z/rWzhWdbo6  I think this should be treated as a GCC+Clang bug.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/from_range.pass.cpp:108
+    operator std::string_view() const noexcept = delete;
+  };
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113161