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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM with an assertion added. Also, bonus points for adding a test for the assertion, but that's non-blocking. Thanks a lot for working on this!



================
Comment at: libcxx/include/string_view:284
+      requires (same_as<iter_value_t<_It>, _CharT> && !convertible_to<_End, size_type>)
+    constexpr _LIBCPP_INLINE_VISIBILITY basic_string_view(_It __begin, _End __end)
+       : __data(_VSTD::to_address(__begin)), __size(__end - __begin) {}
----------------
Those are the same, but `_LIBCPP_HIDE_FROM_ABI` is the preferred name.


================
Comment at: libcxx/include/string_view:287
+    {
+        _LIBCPP_ASSERT(__begin <= __end, "invalid range in string_view's constructor (iterator, iterator)");
+    }
----------------
jloser wrote:
> 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.
I think it's pretty important to add these assertions when we can. And it's a lot easier to do it when we write the function than come back later to add it.

I am working on a patch that will decouple assertions and the debug mode, with the mid/short term goal to allow people to ship libc++ with assertions enabled. So even though it's not super useful today, it will soon be.

I have no strong opinion on adding a `_LIBCPP_ASSERT_VALID_RANGE` macro or doing something like `if constexpr (i-can-order-begin-and-end) { _LIBCPP_ASSERT(...); }`, but an assertion in *some* form seems like a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718