[libcxx-commits] [PATCH] D110718: [libc++] Implement P1391 for string_view

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

Quuxplusone accepted this revision.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/strings/string.view/string.view.cons/deduct.pass.cpp:25
+  auto str = MAKE_STRING(CharT, "test");
+  std::basic_string_view sv{str.begin(), str.end()};
+  ASSERT_SAME_TYPE(decltype(sv), std::basic_string_view<CharT>);
----------------
jloser wrote:
> Quuxplusone wrote:
> > Let's also add `auto sv = std::basic_string_view((const CharT*)str.begin(), str.end());` (with different iterator types), which should also come out to `std::basic_string_view<CharT>`.
> > 
> > Also: Not a blocker necessarily, but I'd strongly prefer to use the `auto sv = std::basic_string_view(str.begin(), str.end());` style for line 25 as well. Using the curly-brace sequence-initialization style makes it look as if we're initializing the `string_view` from a sequence of two elements, and that's not actually what's going on here.
> > 
> > Note that it is physically possible for `auto x = foo{1,2}` and `auto x = foo(1,2)` to wind up using different deduction guides (in fact, this //happens// with `foo`=`std::vector`), so if we really want exhaustive tests we should test both. But I think that's overkill. Then, assuming that we're only going to test one of the two syntaxes, I opine strongly that we should test the one people might ever use, which is the round-parens syntax, not the list-initialization syntax.
> Switched to round-parens syntax. Sorry, just a habit of using braces everywhere as you can tell.
> 
> As for the mixed iterator types, I'm not sure how to make that work with our debug iterators. For example, your proposed test won't work with `__wrap_iter`:
> 
> ```
> error: cannot cast from type 'std::basic_string<char>::iterator' (aka '__wrap_iter<char *>') to pointer type 'const char *'
>     std::basic_string_view sv((const CharT*)str.begin(), str.end());
> ```
> your proposed test won't work with `__wrap_iter`

You could do
```
std::basic_string<CharT> str = MAKE_STRING(CharT, "test");
auto sv = std::basic_string_view(std::as_const(str).begin(), str.end());
```
(or even use `.cbegin()`; I'm not a fan of `.cbegin()` but I admit it'd be shorter than `as_const(...).begin()` in this case).
However, right now you're already testing the deduction guide in `from_iterator_sentinel.pass.cpp`, so maybe just copy-paste that test over into this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718