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

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
Comment at: libcxx/include/string_view:285
+    constexpr _LIBCPP_INLINE_VISIBILITY basic_string_view(_It __begin, _End __end)
+       : __data(_VSTD::to_address(__begin)), __size(static_cast<size_type>(_VSTD::distance(__begin, __end)))
+    {
----------------
Quuxplusone wrote:
> `__size(__end - __begin)`, please. We know the expression is well-formed because we know `_End` is a sized sentinel for `_It`.
Oops, we definitely don't want to do the `std::distance` dance since we have a iterator/sentinel. Just fixed -- thanks!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718