[libcxx-commits] [PATCH] D110718: [libc++] Implement P1391 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/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>);
----------------
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());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718