From 6cd8e5c2031bffab574b62e63bb8e7a7befe4779 Mon Sep 17 00:00:00 2001 From: Hui Date: Sat, 12 Oct 2024 18:24:36 +0100 Subject: [PATCH 1/4] [libc++] Disallow std::prev(non_cpp17_bidi_iterator) --- libcxx/include/__iterator/prev.h | 10 +++++++++- .../iterator.operations/prev.pass.cpp | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/libcxx/include/__iterator/prev.h b/libcxx/include/__iterator/prev.h index 7e97203836eb9..47bfb08950481 100644 --- a/libcxx/include/__iterator/prev.h +++ b/libcxx/include/__iterator/prev.h @@ -17,6 +17,7 @@ #include <__iterator/incrementable_traits.h> #include <__iterator/iterator_traits.h> #include <__type_traits/enable_if.h> +#include <__utility/move.h> #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER) # pragma GCC system_header @@ -26,7 +27,7 @@ _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 = 1) { +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, @@ -35,6 +36,13 @@ prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n = return __x; } +template ::value, int> = 0> +[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _BidirectionalIterator +prev(_BidirectionalIterator __it) { + return std::prev(std::move(__it), 1); +} + #if _LIBCPP_STD_VER >= 20 // [range.iter.op.prev] diff --git a/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp b/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp index 784c1b93b7ca8..be0295d4f7c16 100644 --- a/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp +++ b/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp @@ -18,6 +18,21 @@ #include "test_macros.h" #include "test_iterators.h" +template +std::false_type prev_test(...); + +template +decltype((void) std::prev(std::declval()), std::true_type()) prev_test(int); + +template +using CanPrev = decltype(prev_test(0)); + +static_assert(!CanPrev >::value, ""); +static_assert(CanPrev >::value, ""); +#if TEST_STD_VER >= 20 + static_assert(!CanPrev >::value); +#endif + template TEST_CONSTEXPR_CXX17 void check_prev_n(It it, typename std::iterator_traits::difference_type n, It result) From 0ab15ce7aca36ceec6f2130d059dcffbc748c9c0 Mon Sep 17 00:00:00 2001 From: Hui Date: Sun, 13 Oct 2024 10:44:42 +0100 Subject: [PATCH 2/4] CI --- libcxx/include/__iterator/prev.h | 5 +++++ .../iterator.primitives/iterator.operations/prev.pass.cpp | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/libcxx/include/__iterator/prev.h b/libcxx/include/__iterator/prev.h index 47bfb08950481..d709d0b6787d6 100644 --- a/libcxx/include/__iterator/prev.h +++ b/libcxx/include/__iterator/prev.h @@ -23,6 +23,9 @@ # pragma GCC system_header #endif +_LIBCPP_PUSH_MACROS +#include <__undef_macros> + _LIBCPP_BEGIN_NAMESPACE_STD template ::value, int> = 0> @@ -78,4 +81,6 @@ inline constexpr auto prev = __prev{}; _LIBCPP_END_NAMESPACE_STD +_LIBCPP_POP_MACROS + #endif // _LIBCPP___ITERATOR_PREV_H diff --git a/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp b/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp index be0295d4f7c16..97b9aee704539 100644 --- a/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp +++ b/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp @@ -22,7 +22,7 @@ template std::false_type prev_test(...); template -decltype((void) std::prev(std::declval()), std::true_type()) prev_test(int); +decltype((void)std::prev(std::declval()), std::true_type()) prev_test(int); template using CanPrev = decltype(prev_test(0)); @@ -30,7 +30,7 @@ using CanPrev = decltype(prev_test(0)); static_assert(!CanPrev >::value, ""); static_assert(CanPrev >::value, ""); #if TEST_STD_VER >= 20 - static_assert(!CanPrev >::value); +static_assert(!CanPrev >::value); #endif template From 23deda335755fc782595e4c41feae65b84ef7ad4 Mon Sep 17 00:00:00 2001 From: Hui Date: Sat, 19 Oct 2024 10:15:35 +0100 Subject: [PATCH 3/4] address code review --- libcxx/include/__iterator/prev.h | 8 ++++---- .../iterator.operations/prev.verify.cpp | 19 +++++++++++++++++++ .../iterator.operations/prev.pass.cpp | 15 --------------- 3 files changed, 23 insertions(+), 19 deletions(-) create mode 100644 libcxx/test/libcxx/iterators/iterator.primitives/iterator.operations/prev.verify.cpp diff --git a/libcxx/include/__iterator/prev.h b/libcxx/include/__iterator/prev.h index d709d0b6787d6..e032f6ac75770 100644 --- a/libcxx/include/__iterator/prev.h +++ b/libcxx/include/__iterator/prev.h @@ -39,10 +39,10 @@ prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n) return __x; } -template ::value, int> = 0> -[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _BidirectionalIterator -prev(_BidirectionalIterator __it) { +template ::value, int> = 0> +[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _InputIter prev(_InputIter __it) { + static_assert(__has_bidirectional_iterator_category<_InputIter>::value, + "Attempt to prev(it) with a non-bidirectional iterator"); return std::prev(std::move(__it), 1); } diff --git a/libcxx/test/libcxx/iterators/iterator.primitives/iterator.operations/prev.verify.cpp b/libcxx/test/libcxx/iterators/iterator.primitives/iterator.operations/prev.verify.cpp new file mode 100644 index 0000000000000..da0a336815a5c --- /dev/null +++ b/libcxx/test/libcxx/iterators/iterator.primitives/iterator.operations/prev.verify.cpp @@ -0,0 +1,19 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// std::prev + +#include +#include "test_iterators.h" + +void test() { + int arr[] = {1, 2}; + cpp17_input_iterator it(&arr[0]); + it = std::prev(it); + // expected-error-re@*:* {{static assertion failed due to requirement {{.*}}: Attempt to prev(it) with a non-bidirectional iterator}} +} diff --git a/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp b/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp index 97b9aee704539..784c1b93b7ca8 100644 --- a/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp +++ b/libcxx/test/std/iterators/iterator.primitives/iterator.operations/prev.pass.cpp @@ -18,21 +18,6 @@ #include "test_macros.h" #include "test_iterators.h" -template -std::false_type prev_test(...); - -template -decltype((void)std::prev(std::declval()), std::true_type()) prev_test(int); - -template -using CanPrev = decltype(prev_test(0)); - -static_assert(!CanPrev >::value, ""); -static_assert(CanPrev >::value, ""); -#if TEST_STD_VER >= 20 -static_assert(!CanPrev >::value); -#endif - template TEST_CONSTEXPR_CXX17 void check_prev_n(It it, typename std::iterator_traits::difference_type n, It result) From f5289a1422756913ffe4d7daea378cdd268a42b4 Mon Sep 17 00:00:00 2001 From: Hui Date: Sat, 19 Oct 2024 10:21:10 +0100 Subject: [PATCH 4/4] add comment --- libcxx/include/__iterator/prev.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libcxx/include/__iterator/prev.h b/libcxx/include/__iterator/prev.h index e032f6ac75770..bffd5527dc953 100644 --- a/libcxx/include/__iterator/prev.h +++ b/libcxx/include/__iterator/prev.h @@ -39,6 +39,9 @@ prev(_InputIter __x, typename iterator_traits<_InputIter>::difference_type __n) return __x; } +// LWG 3197 +// It is unclear what the implications of "BidirectionalIterator" in the standard are. +// However, calling std::prev(non-bidi-iterator) is obviously an error and we should catch it at compile time. template ::value, int> = 0> [[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _InputIter prev(_InputIter __it) { static_assert(__has_bidirectional_iterator_category<_InputIter>::value,