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

Wed Oct 6 06:58:04 PDT 2021

jloser marked 4 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:
> 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.
Added an assert with `_LIBCPP_ASSERT((__end - __begin) >= 0)`. 

================
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) {}
----------------
ldionne wrote:
> Those are the same, but `_LIBCPP_HIDE_FROM_ABI` is the preferred name.
Renamed. I used `_LIBCPP_INLINE_VISIBILITY` originally since it's what is used in the rest of this file. WDYT about a global `s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/g`? And then we just adjust `_LIBCPP_HIDE_FROM_ABI` macro definition to not be defined in terms of `_LIBCPP_INLINE_VISIBILITY` but instead just be

```
#define _LIBCPP_HIDE_FROM_ABI __attribute__ ((__visibility__("hidden"), __always_inline__))
```

Then we avoid this issue in the future of having two identifiers to do the same thing.

================
Comment at: libcxx/test/support/test_iterators.h:919
+  return iter - sent.base();
+}
+
----------------
Quuxplusone wrote:
> 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>`.)
Renamed to `sized_sentinel`. It's unfortunate that `sentinel` is such a common name used in the range tests so it's off the table without a bit of work for renaming `sentinel_wrapper` to `sentinel`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110718