[libcxx-commits] [PATCH] D108827: [libc++] Add an assertion in the subrange constructors with a size hint

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/subrange.h:123-125
+      if constexpr (sized_sentinel_for<_Sent, decltype(__iter)>)
+        _LIBCPP_ASSERT(ranges::distance(this->__begin_, this->__end_) == __n, "std::ranges::subrange was passed an invalid size hint");
+    }
----------------
ldionne wrote:
> Quuxplusone wrote:
> > It is mildly sketchy that you're testing `sized_sentinel_for` on `decltype(__iter)` (which is not `_Iter`) but then invoking `ranges::distance` on `_Iter`. What if `!sized_sentinel_for<_Sent, _Iter>`?
> > 
> > Also, what if `!forward_iterator<_Iter>`? Then it is invalid to mess with `this->__begin_` except at the user's command.
> > 
> > Also, what if `!copyable<_Iter>`? I don't think that's possible today, but I think there are active proposals for move-only iterators. (But even with those proposals, I bet `forward_iterator<_Iter>` implies `copyable<_Iter>`. So that's probably fine.)
> > 
> > I suspect that this is a UB-if-violated "Precondition", not a "Mandates", on purpose, and we should just leave it alone.
> > It is mildly sketchy that you're testing sized_sentinel_for on decltype(__iter) (which is not _Iter) but then invoking ranges::distance on _Iter. What if !sized_sentinel_for<_Sent, _Iter>?
> 
> Right, this should be `if constexpr (sized_sentinel_for<_Sent, _Iter>)` instead. Initially I used `ranges::distance(__iter, __sent)` so this was correct, but then I noticed we were moving from `__iter` so I changed to `this->__begin_` and forgot to update the `if constexpr`. Will fix.
> 
> > Also, what if `!forward_iterator<_Iter>`? Then it is invalid to mess with `this->__begin_` except at the user's command.
> 
> Ugh, sure. My thinking was that if you model `sized_sentinel_for`, clearly you don't expect that evaluating `iter - sent` is going to do something crazy like invalidate your range. I'm even fine with adding `random_access_iterator<_Iter>` -- the goal here really is to do our due diligence at catching the UB when it's easy to do that.
> 
> > I suspect that this is a UB-if-violated "Precondition", not a "Mandates", on purpose, and we should just leave it alone.
> 
> I disagree pretty strongly. It's easy to check this precondition, and if the programmer got it wrong, they might have a more serious bug in their program. I think the least we can do is add an assertion to help them catch the issue.
Well, now the `if` is no longer testing the condition from the Standard wording, so we're drifting away from "checking the precondition" and into "checking a class invariant." I think I agree that what you've got now checks a valid class invariant for this class.
However, for simplicity, how about we do this instead?
```
if constexpr (sized_sentinel_for<_Sent, _Iter>)
    _LIBCPP_ASSERT((this->__end_ - this->__begin_) == iter_difference_t<_Iter>(this->__size_), "std::ranges::subrange was passed a size hint with the wrong value");
```
That is, whenever `end - begin` is well-formed, the `size` that is passed in must be equal to `end - begin`.
I put the cast on `__size_` just to avoid comparing signed with unsigned. `__size_` and `__n` should be equal at this point.

I think it's reasonable to assume that evaluating `end - begin` will not invalidate `begin`, so we no longer need to check for `forward_iterator<_Iter>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108827