[libcxx-commits] [libcxx] a5d919b - [libc++] Disallow std::prev(non_cpp17_bidi_iterator) (#112102)

[via libcxx-commits]

Author: Hui
Date: 2024-10-23T14:42:15-04:00
New Revision: a5d919b4b2f24c582838659fe7f30073e7285524

URL: https://github.com/llvm/llvm-project/commit/a5d919b4b2f24c582838659fe7f30073e7285524
DIFF: https://github.com/llvm/llvm-project/commit/a5d919b4b2f24c582838659fe7f30073e7285524.diff

LOG: [libc++] Disallow std::prev(non_cpp17_bidi_iterator) (#112102)

The std::prev function appeared to work on non Cpp17BidirectionalIterators, however it
behaved strangely by being the identity function. That was extremely misleading and
potentially dangerous, since several recent iterators that are C++20 bidirectional_iterators
don't satisfy the Cpp17BidirectionalIterator named requirement. For example:

    auto zv = std::views::zip(vec1, vec2);
    auto it = zv.begin() + 5;
    auto it2 = std::prev(it); // "it2" will be the same as "it",  instead of the previous one

Here, zip_view::iterator is a c++20 random_access_iterator, but it only satisfies the
Cpp17InputIterator named requirement because its reference type is a proxy type. Hence
`std::prev` would silently accept that iterator but it would do a no-op instead of going
to the previous position.

This patch changes `std::prev(it)` to produce an error at compile-time when instantiated
with a type that is not a Cpp17BidirectionalIterator.

Fixes #109456

Added: 
    libcxx/test/libcxx/iterators/iterator.primitives/iterator.operations/prev.verify.cpp

Modified: 
    libcxx/include/__iterator/prev.h

Removed: 
    


################################################################################
diff  --git a/libcxx/include/__iterator/prev.h b/libcxx/include/__iterator/prev.h
index 7e97203836eb98..bffd5527dc953e 100644
--- a/libcxx/include/__iterator/prev.h
+++ b/libcxx/include/__iterator/prev.h
@@ -17,16 +17,20 @@
 #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
 #endif
 
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _InputIter, __enable_if_t<__has_input_iterator_category<_InputIter>::value, int> = 0>
 [[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX17 _InputIter
-prev(_InputIter __x, typename iterator_traits<_InputIter>::
diff erence_type __n = 1) {
+prev(_InputIter __x, typename iterator_traits<_InputIter>::
diff erence_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 +39,16 @@ prev(_InputIter __x, typename iterator_traits<_InputIter>::
diff erence_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 <class _InputIter, __enable_if_t<__has_input_iterator_category<_InputIter>::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);
+}
+
 #if _LIBCPP_STD_VER >= 20
 
 // [range.iter.op.prev]
@@ -70,4 +84,6 @@ inline constexpr auto prev = __prev{};
 
 _LIBCPP_END_NAMESPACE_STD
 
+_LIBCPP_POP_MACROS
+
 #endif // _LIBCPP___ITERATOR_PREV_H

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 00000000000000..da0a336815a5c8
--- /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 <iterator>
+#include "test_iterators.h"
+
+void test() {
+  int arr[] = {1, 2};
+  cpp17_input_iterator<int*> 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}}
+}