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

[Joe Loser via Phabricator via libcxx-commits]

jloser marked 3 inline comments as done.
jloser added inline comments.


================
Comment at: libcxx/include/string_view:287
+    {
+        _LIBCPP_ASSERT(__begin <= __end, "invalid range in string_view's constructor (iterator, iterator)");
+    }
----------------
Quuxplusone wrote:
> jloser wrote:
> > Quuxplusone wrote:
> > > Vice versa, I see no reason for `__begin <= __end` to be valid. Please add a test case using a non-iterator sentinel type. (I think my SFINAE test below is //not// sufficient, because it won't instantiate the body of this template, and it's the //body// that explodes.)
> > Well, the standard states that `[begin, end) is a valid range` as a precondition. I noticed we have some inconsistencies with `string` and `string_view` when it comes to these precondition checking. For the most part, we don't seem to actually check valid ranges, but we do in `std::string::erase` (https://github.com/llvm/llvm-project/blob/fd9bc13803ee24bfa674311584126b83e059d756/libcxx/include/string#L3228)
> > 
> > What's the recommended advice here @ldionne? From my perspective, a friendly debug `_LIBCPP_ASSERT` leads to a higher quality implementation given that the standard states the precondition explicitly. 
> > 
> > For consistency, I'll update the message to say `(iterator, sentinel)` instead of `(iterator, iterator)`, but let's decide if we want to have it at all first.
> I think either we shouldn't have the assert, or (if we do) we should hide it behind a new macro, something like `_LIBCPP_ASSERT_VALID_RANGE(__begin, __end)`. Then all the magic can go inside that macro and be reused consistently — whether it's `requires`, or calling `_VSTD::distance`, or what. Plus (although Louis will hate this idea ;)) anyone who wants ordinary assertions without paying for valid-range assertions can just compile with `-D_LIBCPP_ASSERT_VALID_RANGE(x,y)=(void)0`.
I'd be in favor of the assert personally, but I don't feel like pushing for it in this PR. I like the idea of factoring it out into a macro as I can see valid range checks being used all over in `libc++` preconditions.

I don't imagine many people **actually** (except @Quuxplusone of course :)) wanting all-but-range-check assertions.


We'll see what @ldionne says, but nothing to block this PR on I'd imagine.


================
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());
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718