[libcxx-commits] [libcxx] [libc++] `std::ranges::advance`: avoid unneeded bounds checks when advancing iterator (PR #84126)

[Jan Kokemüller via libcxx-commits]

================
@@ -213,6 +221,20 @@ constexpr bool test() {
     assert(i == iota_iterator{INT_MIN+1});
   }
 
+  // Check that we don't do an unneeded bounds check when decrementing a
----------------
jiixyj wrote:

Yes, that comment does not seem to be correct.

My intention with the `!sized_sentinel_for` check was to _not_ fall into this case: <https://eel.is/c++draft/iterators#range.iter.op.advance-6.1>, but instead test the other one: <https://eel.is/c++draft/iterators#range.iter.op.advance-6.2>.

But it should actually be possible to remove the new, "special case" test and instead improve the generic `check_backward` tests, right? Because as you mentioned, being `sized_sentinel_for<It, It>` is not actually a precondition for "`advance` negative `n`". In an improved `check_backward` test, one would only have to ensure the actual preconditions:

- `[bound, i)` is a range
- that `It` is a `std::bidirectional_iterator`
- that `S` (the sentinel type) is the same as `It`

Then, one could instantiate `check_backward` with different iterator types that either model `sized_sentinel_for<It, It>` or don't. This way it would be possible to get coverage for both ["range.iter.op.advance-6.1"](<https://eel.is/c++draft/iterators#range.iter.op.advance-6.1>) and ["range.iter.op.advance-6.2"](<https://eel.is/c++draft/iterators#range.iter.op.advance-6.2>). Does this sound sensible to you?

https://github.com/llvm/llvm-project/pull/84126