[libcxx-commits] [PATCH] D110718: [libc++] Implement P1391 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/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:
> 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.
Added tests for mixed const iterators in the `deduct.pass.cpp` by just copying the `test` function template from `from_iterator_sentinel_pass.cpp`.

Also changed `from_iterator_sentinel.pass.cpp` to not rely on the deduction guide. Otherwise, `deduct.pass` is completely superfluous with the old `from_iterator_sentinel.pass.cpp` that used the deduction guide.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718