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

[Konstantin Varlamov via libcxx-commits]

Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>,
Jan =?utf-8?q?Kokemüller?= <jan.kokemueller at gmail.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/84126 at github.com>


================
@@ -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
----------------
var-const wrote:

I'm not sure why the comment says that being a sized sentinel is a requirement -- at least in the [current draft](http://eel.is/c++draft/iterators#range.iter.op.advance-5) of the Standard, all I see is a requirement that the iterator and the sentinel are the same type. IIUC, if the comment were true, the new test case would actually be UB (because it calls `advance` with a negative `n` and a bidirectional iterator).

Assuming the comment is not correct, IMO the `!sized_sentinel_for` check creates more confusion than clarity. The bidirectional iterator concept doesn't require the type to be a sized sentinel for itself, and in tests we use "minimal" iterator types (types that only support the bare minimum operations required to satisfy the concept). So I think this would be clearer if we simply test with a bidirectional iterator and omit the `static_assert`s and the comment.

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