[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/from_iterator_sentinel.pass.cpp:24-30
+template<class CharT>
+constexpr void test() {
+  auto val = MAKE_STRING_VIEW(CharT, "test");
+  std::basic_string_view<CharT> sv{val.begin(), val.end()};
+  assert(sv.size() == val.size());
+  assert(sv.data() == val.data());
+}
----------------
jloser wrote:
> Quuxplusone wrote:
> > I suggest
> > ```
> > template<class CharT, class Sentinel>
> > constexpr void test() {
> >   auto val = MAKE_STRING_VIEW(CharT, "test");
> >   auto sv = std::basic_string_view(val.begin(), Sentinel(val.end()));
> >   ASSERT_SAME_TYPE(decltype(sv), std::basic_string_view<CharT>);
> >   assert(sv.size() == val.size());
> >   assert(sv.data() == val.data());
> > }
> > ```
> > and then test with
> > ```
> > test<char, char*>();
> > test<wchar_t, wchar_t*>();
> > test<char8_t, char8_t*>();
> > test<char16_t, char16_t*>();
> > test<char32_t, char32_t*>();
> > test<char, const char*>();
> > test<char, sized_sentinel_wrapper<char*>>();
> > ```
> > On the last line, `sized_sentinel_wrapper` would go into `test_iterators.h`, right after `sentinel_wrapper`; it would just provide `operator-` in addition to the `sentinel_wrapper` stuff... ick, actually, I see several existing tests overloading `operator-` for `sentinel_wrapper` in incompatible ways. I'll clean that up in a new PR myself. Needn't block this on that, although, I won't complain if you do. ;)
> Added a test using a locally defined sentinel with appropriate `operator==` and `operator-`. I played for a bit in `test_iterators.h` with a `sized_sentinel_wrapper`. However, things didn't play so nicely due to constness and other things. Perhaps I overlooked something, but I'd say let's refactor this new use among others to use the new `sized_sentinel_wrapper` in a follow-up PR. I'm interested to see your PR for cleaning up the `operator--` usage.
> 
> In the refactor, we can clean up the other uses in `libcxx/test/std/ranges/range.adaptors/range.common.view/types.h` and `libcxx/test/std/ranges/range.adaptors/range.take/types.h` from a quick look around.
@Quuxplusone just reworked this to use `sized_sentinel_wrapper` and things worked out nicely this time around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718