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

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

Quuxplusone accepted this revision.
Quuxplusone added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/string_view:287
+    {
+        _LIBCPP_ASSERT(__begin <= __end, "invalid range in string_view's constructor (iterator, iterator)");
+    }
----------------
ldionne wrote:
> 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.
I strongly prefer a macro over `if constexpr`-anything.
However, it occurs to me that even though `_LIBCPP_ASSERT(__begin <= __end)` would be ill-formed, we could still simply do `_LIBCPP_ASSERT((__end - __begin) >= 0)`. I don't object if we want to add that, and I don't think it needs to be hidden behind a macro.


================
Comment at: libcxx/test/support/test_iterators.h:919
+  return iter - sent.base();
+}
+
----------------
If I weren't working on a PR for this, I would have some style comments here; but as I //will// eventually post a PR to clean this up, I don't object to this for now.
The one thing that would be helpful to do now, if it's easy enough for you, would be to rename `sized_sentinel_wrapper<It>` to just `sized_sentinel<It>`. Unfortunately we can't trivially rename `sentinel_wrapper<It>` to just `sentinel<It>` because the global name `sentinel` has already been land-grabbed by a few tests; but we can use the shorter and more consistent name `sized_sentinel` no problem. (Note that we use e.g. `random_access_iterator<It>`, not `random_access_iterator_wrapper<It>`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718