From d05ab119e188be99697f142a02c6b71137fde082 Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Tue, 6 May 2025 15:44:26 -0400 Subject: [PATCH] [libc++] Remove redundant and somewhat confusing assertions around advance() (#133276) The std::advance function has a clear precondition that it can only be called with a negative distance when a bidirectional iterator is used. However, prev() and next() don't have such preconditions explicitly, they inherit it from calling advance(). This patch removes assertions in prev() and next() that were duplicates of similar ones in advance(), and removes a copy-pasted comment that was trying to justify the use of _LIBCPP_ASSERT_PEDANTIC but IMO is creating confusion with little benefit. --- libcxx/include/__iterator/advance.h | 14 ++++++-------- libcxx/include/__iterator/next.h | 6 ------ libcxx/include/__iterator/prev.h | 5 ----- libcxx/test/libcxx/iterators/assert.next.pass.cpp | 2 +- libcxx/test/libcxx/iterators/assert.prev.pass.cpp | 2 +- 5 files changed, 8 insertions(+), 21 deletions(-) diff --git a/libcxx/include/__iterator/advance.h b/libcxx/include/__iterator/advance.h index 57b1b845f1af..f1a8d28f39aa 100644 --- a/libcxx/include/__iterator/advance.h +++ b/libcxx/include/__iterator/advance.h @@ -65,8 +65,7 @@ template < class _InputIter, _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 void advance(_InputIter& __i, _Distance __orig_n) { typedef typename iterator_traits<_InputIter>::difference_type _Difference; _Difference __n = static_cast<_Difference>(std::__convert_to_integral(__orig_n)); - // Calling `advance` with a negative value on a non-bidirectional iterator is a no-op in the current implementation. - _LIBCPP_ASSERT_PEDANTIC(__n >= 0 || __has_bidirectional_iterator_category<_InputIter>::value, + _LIBCPP_ASSERT_PEDANTIC(__has_bidirectional_iterator_category<_InputIter>::value || __n >= 0, "Attempt to advance(it, n) with negative n on a non-bidirectional iterator"); std::__advance(__i, __n, typename iterator_traits<_InputIter>::iterator_category()); } @@ -98,9 +97,8 @@ public: // Preconditions: If `I` does not model `bidirectional_iterator`, `n` is not negative. template _LIBCPP_HIDE_FROM_ABI constexpr void operator()(_Ip& __i, iter_difference_t<_Ip> __n) const { - // Calling `advance` with a negative value on a non-bidirectional iterator is a no-op in the current implementation. - _LIBCPP_ASSERT_PEDANTIC( - __n >= 0 || bidirectional_iterator<_Ip>, "If `n < 0`, then `bidirectional_iterator` must be true."); + _LIBCPP_ASSERT_PEDANTIC(bidirectional_iterator<_Ip> || __n >= 0, + "ranges::advance: Can only pass a negative `n` with a bidirectional_iterator."); // If `I` models `random_access_iterator`, equivalent to `i += n`. if constexpr (random_access_iterator<_Ip>) { @@ -149,9 +147,9 @@ public: template _Sp> _LIBCPP_HIDE_FROM_ABI constexpr iter_difference_t<_Ip> operator()(_Ip& __i, iter_difference_t<_Ip> __n, _Sp __bound_sentinel) const { - // Calling `advance` with a negative value on a non-bidirectional iterator is a no-op in the current implementation. - _LIBCPP_ASSERT_PEDANTIC((__n >= 0) || (bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>), - "If `n < 0`, then `bidirectional_iterator && same_as` must be true."); + _LIBCPP_ASSERT_PEDANTIC( + (bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>) || (__n >= 0), + "ranges::advance: Can only pass a negative `n` with a bidirectional_iterator coming from a common_range."); // If `S` and `I` model `sized_sentinel_for`: if constexpr (sized_sentinel_for<_Sp, _Ip>) { // If |n| >= |bound_sentinel - i|, equivalent to `ranges::advance(i, bound_sentinel)`. diff --git a/libcxx/include/__iterator/next.h b/libcxx/include/__iterator/next.h index 1f68a5bec8f3..1143ab31ff7c 100644 --- a/libcxx/include/__iterator/next.h +++ b/libcxx/include/__iterator/next.h @@ -10,7 +10,6 @@ #ifndef _LIBCPP___ITERATOR_NEXT_H #define _LIBCPP___ITERATOR_NEXT_H -#include <__assert> #include <__config> #include <__iterator/advance.h> #include <__iterator/concepts.h> @@ -27,11 +26,6 @@ _LIBCPP_BEGIN_NAMESPACE_STD template ::value, int> = 0> [[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _InputIter next(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n = 1) { - // Calling `advance` with a negative value on a non-bidirectional iterator is a no-op in the current implementation. - // Note that this check duplicates the similar check in `std::advance`. - _LIBCPP_ASSERT_PEDANTIC(__n >= 0 || __has_bidirectional_iterator_category<_InputIter>::value, - "Attempt to next(it, n) with negative n on a non-bidirectional iterator"); - std::advance(__x, __n); return __x; } diff --git a/libcxx/include/__iterator/prev.h b/libcxx/include/__iterator/prev.h index bffd5527dc95..97139e067c89 100644 --- a/libcxx/include/__iterator/prev.h +++ b/libcxx/include/__iterator/prev.h @@ -10,7 +10,6 @@ #ifndef _LIBCPP___ITERATOR_PREV_H #define _LIBCPP___ITERATOR_PREV_H -#include <__assert> #include <__config> #include <__iterator/advance.h> #include <__iterator/concepts.h> @@ -31,10 +30,6 @@ _LIBCPP_BEGIN_NAMESPACE_STD template ::value, int> = 0> [[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _InputIter prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n) { - // Calling `advance` with a negative value on a non-bidirectional iterator is a no-op in the current implementation. - // Note that this check duplicates the similar check in `std::advance`. - _LIBCPP_ASSERT_PEDANTIC(__n <= 0 || __has_bidirectional_iterator_category<_InputIter>::value, - "Attempt to prev(it, n) with a positive n on a non-bidirectional iterator"); std::advance(__x, -__n); return __x; } diff --git a/libcxx/test/libcxx/iterators/assert.next.pass.cpp b/libcxx/test/libcxx/iterators/assert.next.pass.cpp index f6fd24284bbf..1e8672308554 100644 --- a/libcxx/test/libcxx/iterators/assert.next.pass.cpp +++ b/libcxx/test/libcxx/iterators/assert.next.pass.cpp @@ -25,7 +25,7 @@ int main(int, char**) { forward_iterator it(a+1); (void)std::next(it, 1); // should work fine (void)std::next(it, 0); // should work fine - TEST_LIBCPP_ASSERT_FAILURE(std::next(it, -1), "Attempt to next(it, n) with negative n on a non-bidirectional iterator"); + TEST_LIBCPP_ASSERT_FAILURE(std::next(it, -1), "Attempt to advance(it, n) with negative n on a non-bidirectional iterator"); return 0; } diff --git a/libcxx/test/libcxx/iterators/assert.prev.pass.cpp b/libcxx/test/libcxx/iterators/assert.prev.pass.cpp index 08cbe5e03dd5..29b8d6ed5d1e 100644 --- a/libcxx/test/libcxx/iterators/assert.prev.pass.cpp +++ b/libcxx/test/libcxx/iterators/assert.prev.pass.cpp @@ -31,7 +31,7 @@ int main(int, char**) { forward_iterator it(a+1); (void)std::prev(it, -1); // should work fine (void)std::prev(it, 0); // should work fine - TEST_LIBCPP_ASSERT_FAILURE(std::prev(it, 1), "Attempt to prev(it, n) with a positive n on a non-bidirectional iterator"); + TEST_LIBCPP_ASSERT_FAILURE(std::prev(it, 1), "Attempt to advance(it, n) with negative n on a non-bidirectional iterator"); return 0; }