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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/string_view:287
+    {
+        _LIBCPP_ASSERT(__begin <= __end, "invalid range in string_view's constructor (iterator, iterator)");
+    }
----------------
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`.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718