From 12b01ab7fa10939d67ac7cb2da1d3ca8a41b5fcd Mon Sep 17 00:00:00 2001 From: Louis Dionne Date: Mon, 8 Jun 2020 16:16:01 -0400 Subject: [PATCH] [libc++] Don't trigger unsigned conversion warnings in std::advance The Standard documents the signature of std::advance as template constexpr void advance(Iter& i, Distance n); Furthermore, it does not appear to put any restriction on what the type of Distance should be. While it is understood that it should usually be std::iterator_traits::difference_type, I couldn't find any wording that mandates that. Similarly, I couldn't find wording that forces the distance to be a signed type. This patch changes std::advance to accept any type in the second argument, which appears to be what the Standard mandates. We then coerce it to the iterator's difference type, but that's an implementation detail. Differential Revision: https://reviews.llvm.org/D81425 --- libcxx/include/iterator | 21 +++++---- .../iterator.operations/advance.pass.cpp | 43 ++++++++++++------- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/libcxx/include/iterator b/libcxx/include/iterator index 57dd055b4ac9..a13214fca5e4 100644 --- a/libcxx/include/iterator +++ b/libcxx/include/iterator @@ -54,10 +54,8 @@ struct bidirectional_iterator_tag : public forward_iterator_tag {}; struct random_access_iterator_tag : public bidirectional_iterator_tag {}; // 27.4.3, iterator operations -// extension: second argument not conforming to C++03 -template // constexpr in C++17 - constexpr void advance(InputIterator& i, - typename iterator_traits::difference_type n); +template // constexpr in C++17 + constexpr void advance(InputIterator& i, Distance n); template // constexpr in C++17 constexpr typename iterator_traits::difference_type @@ -663,13 +661,14 @@ void __advance(_RandIter& __i, __i += __n; } -template +template inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 -void advance(_InputIter& __i, - typename iterator_traits<_InputIter>::difference_type __n) +void advance(_InputIter& __i, _Distance __orig_n) { - _LIBCPP_ASSERT(__n >= 0 || __is_cpp17_bidirectional_iterator<_InputIter>::value, - "Attempt to advance(it, -n) on a non-bidi iterator"); + _LIBCPP_ASSERT(__orig_n >= 0 || __is_cpp17_bidirectional_iterator<_InputIter>::value, + "Attempt to advance(it, n) with negative n on a non-bidirectional iterator"); + typedef decltype(__convert_to_integral(__orig_n)) _IntegralSize; + _IntegralSize __n = __orig_n; __advance(__i, __n, typename iterator_traits<_InputIter>::iterator_category()); } @@ -711,7 +710,7 @@ next(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n = 1) { _LIBCPP_ASSERT(__n >= 0 || __is_cpp17_bidirectional_iterator<_InputIter>::value, - "Attempt to next(it, -n) on a non-bidi iterator"); + "Attempt to next(it, n) with negative n on a non-bidirectional iterator"); _VSTD::advance(__x, __n); return __x; @@ -728,7 +727,7 @@ prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n = 1) { _LIBCPP_ASSERT(__n <= 0 || __is_cpp17_bidirectional_iterator<_InputIter>::value, - "Attempt to prev(it, +n) on a non-bidi iterator"); + "Attempt to prev(it, n) with a positive n on a non-bidirectional iterator"); _VSTD::advance(__x, -__n); return __x; } diff --git a/libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp b/libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp index c0dd85c783f1..75b86e3d3e89 100644 --- a/libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp +++ b/libcxx/test/std/iterators/iterator.primitives/iterator.operations/advance.pass.cpp @@ -10,25 +10,29 @@ // All of these became constexpr in C++17 // -// template -// constexpr void advance(Iter& i, Iter::difference_type n); +// template +// constexpr void advance(Iter& i, Distance n); // -// template -// constexpr void advance(Iter& i, Iter::difference_type n); +// template +// constexpr void advance(Iter& i, Distance n); // -// template -// constexpr void advance(Iter& i, Iter::difference_type n); +// template +// constexpr void advance(Iter& i, Distance n); + +// Make sure we catch forced conversions to the difference_type if they happen. +// ADDITIONAL_COMPILER_FLAGS: -Wsign-conversion #include #include +#include #include #include "test_macros.h" #include "test_iterators.h" -template +template TEST_CONSTEXPR_CXX17 -void check_advance(It it, typename std::iterator_traits::difference_type n, It result) +void check_advance(It it, Distance n, It result) { static_assert(std::is_same::value, ""); std::advance(it, n); @@ -38,14 +42,21 @@ void check_advance(It it, typename std::iterator_traits::difference_type n, TEST_CONSTEXPR_CXX17 bool tests() { const char* s = "1234567890"; - check_advance(input_iterator(s), 10, input_iterator(s+10)); - check_advance(forward_iterator(s), 10, forward_iterator(s+10)); - check_advance(bidirectional_iterator(s+5), 5, bidirectional_iterator(s+10)); - check_advance(bidirectional_iterator(s+5), -5, bidirectional_iterator(s)); - check_advance(random_access_iterator(s+5), 5, random_access_iterator(s+10)); - check_advance(random_access_iterator(s+5), -5, random_access_iterator(s)); - check_advance(s+5, 5, s+10); - check_advance(s+5, -5, s); + typedef std::iterator_traits::difference_type Distance; + check_advance(input_iterator(s), 10, input_iterator(s+10)); + check_advance(forward_iterator(s), 10, forward_iterator(s+10)); + check_advance(bidirectional_iterator(s+5), 5, bidirectional_iterator(s+10)); + check_advance(bidirectional_iterator(s+5), -5, bidirectional_iterator(s)); + check_advance(random_access_iterator(s+5), 5, random_access_iterator(s+10)); + check_advance(random_access_iterator(s+5), -5, random_access_iterator(s)); + check_advance(s+5, 5, s+10); + check_advance(s+5, -5, s); + + // Also check with other distance types + check_advance(input_iterator(s), 10u, input_iterator(s+10)); + check_advance(forward_iterator(s), 10u, forward_iterator(s+10)); + check_advance(bidirectional_iterator(s), 10u, bidirectional_iterator(s+10)); + check_advance(random_access_iterator(s), 10u, random_access_iterator(s+10)); return true; }