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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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");
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108827