Revert "Add make_value_iterator (#3271)"

This reverts commit ee0c5ee.

Author: henryiii

docs/reference.rst

@@ -63,9 +63,6 @@ Convenience functions converting to Python types
 .. doxygenfunction:: make_key_iterator(Iterator, Sentinel, Extra &&...)
 .. doxygenfunction:: make_key_iterator(Type &, Extra&&...)
 
-.. doxygenfunction:: make_value_iterator(Iterator, Sentinel, Extra &&...)
-.. doxygenfunction:: make_value_iterator(Type &, Extra&&...)
-
 .. _extras:
 
 Passing extra arguments to ``def`` or ``class_``

include/pybind11/pybind11.h

@@ -1955,52 +1955,25 @@ inline std::pair<decltype(internals::registered_types_py)::iterator, bool> all_t
     return res;
 }
 
-/* There are a large number of apparently unused template arguments because
- * each combination requires a separate py::class_ registration.
- */
-template <typename Access, return_value_policy Policy, typename Iterator, typename Sentinel, typename ValueType, typename... Extra>
+template <typename Iterator, typename Sentinel, bool KeyIterator, return_value_policy Policy>
 struct iterator_state {
     Iterator it;
     Sentinel end;
     bool first_or_done;
 };
 
-// Note: these helpers take the iterator by non-const reference because some
-// iterators in the wild can't be dereferenced when const.
-template <typename Iterator>
-struct iterator_access {
-    using result_type = decltype((*std::declval<Iterator>()));
-    // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
-    result_type operator()(Iterator &it) const {
-        return *it;
-    }
-};
-
-template <typename Iterator>
-struct iterator_key_access {
-    using result_type = decltype(((*std::declval<Iterator>()).first));
-    result_type operator()(Iterator &it) const {
-        return (*it).first;
-    }
-};
-
-template <typename Iterator>
-struct iterator_value_access {
-    using result_type = decltype(((*std::declval<Iterator>()).second));
-    result_type operator()(Iterator &it) const {
-        return (*it).second;
-    }
-};
+PYBIND11_NAMESPACE_END(detail)
 
-template <typename Access,
-          return_value_policy Policy,
+/// Makes a python iterator from a first and past-the-end C++ InputIterator.
+template <return_value_policy Policy = return_value_policy::reference_internal,
           typename Iterator,
           typename Sentinel,
-          typename ValueType,
+#ifndef DOXYGEN_SHOULD_SKIP_THIS  // Issue in breathe 4.26.1
+          typename ValueType = decltype(*std::declval<Iterator>()),
+#endif
           typename... Extra>
-iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&... extra) {
-    using state = detail::iterator_state<Access, Policy, Iterator, Sentinel, ValueType, Extra...>;
-    // TODO: state captures only the types of Extra, not the values
+iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
+    using state = detail::iterator_state<Iterator, Sentinel, false, Policy>;
 
     if (!detail::get_type_info(typeid(state), false)) {
         class_<state>(handle(), "iterator", pybind11::module_local())
@@ -2014,63 +1987,43 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&... extra) {
                     s.first_or_done = true;
                     throw stop_iteration();
                 }
-                return Access()(s.it);
+                return *s.it;
             // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
             }, std::forward<Extra>(extra)..., Policy);
     }
 
     return cast(state{first, last, true});
 }
 
-PYBIND11_NAMESPACE_END(detail)
-
-/// Makes a python iterator from a first and past-the-end C++ InputIterator.
-template <return_value_policy Policy = return_value_policy::reference_internal,
-          typename Iterator,
-          typename Sentinel,
-          typename ValueType = typename detail::iterator_access<Iterator>::result_type,
-          typename... Extra>
-iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
-    return detail::make_iterator_impl<
-        detail::iterator_access<Iterator>,
-        Policy,
-        Iterator,
-        Sentinel,
-        ValueType,
-        Extra...>(first, last, std::forward<Extra>(extra)...);
-}
-
-/// Makes a python iterator over the keys (`.first`) of a iterator over pairs from a
+/// Makes an python iterator over the keys (`.first`) of a iterator over pairs from a
 /// first and past-the-end InputIterator.
 template <return_value_policy Policy = return_value_policy::reference_internal,
           typename Iterator,
           typename Sentinel,
-          typename KeyType = typename detail::iterator_key_access<Iterator>::result_type,
+#ifndef DOXYGEN_SHOULD_SKIP_THIS  // Issue in breathe 4.26.1
+          typename KeyType = decltype((*std::declval<Iterator>()).first),
+#endif
           typename... Extra>
 iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) {
-    return detail::make_iterator_impl<
-        detail::iterator_key_access<Iterator>,
-        Policy,
-        Iterator,
-        Sentinel,
-        KeyType,
-        Extra...>(first, last, std::forward<Extra>(extra)...);
-}
+    using state = detail::iterator_state<Iterator, Sentinel, true, Policy>;
 
-/// Makes a python iterator over the values (`.second`) of a iterator over pairs from a
-/// first and past-the-end InputIterator.
-template <return_value_policy Policy = return_value_policy::reference_internal,
-          typename Iterator,
-          typename Sentinel,
-          typename ValueType = typename detail::iterator_value_access<Iterator>::result_type,
-          typename... Extra>
-iterator make_value_iterator(Iterator first, Sentinel last, Extra &&...extra) {
-    return detail::make_iterator_impl<
-        detail::iterator_value_access<Iterator>,
-        Policy, Iterator,
-        Sentinel,
-        ValueType,
-        Extra...>(first, last, std::forward<Extra>(extra)...);
+    if (!detail::get_type_info(typeid(state), false)) {
+        class_<state>(handle(), "iterator", pybind11::module_local())
+            .def("__iter__", [](state &s) -> state& { return s; })
+            .def("__next__", [](state &s) -> detail::remove_cv_t<KeyType> {
+                if (!s.first_or_done)
+                    ++s.it;
+                else
+                    s.first_or_done = false;
+                if (s.it == s.end) {
+                    s.first_or_done = true;
+                    throw stop_iteration();
+                }
+                return (*s.it).first;
+            }, std::forward<Extra>(extra)..., Policy);
+    }
+
+    return cast(state{first, last, true});
 }
 
 /// Makes an iterator over values of an stl container or other container supporting
@@ -2087,13 +2040,6 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
     return make_key_iterator<Policy>(std::begin(value), std::end(value), extra...);
 }
 
-/// Makes an iterator over the values (`.second`) of a stl map-like container supporting
-/// `std::begin()`/`std::end()`
-template <return_value_policy Policy = return_value_policy::reference_internal,
-          typename Type, typename... Extra> iterator make_value_iterator(Type &value, Extra&&... extra) {
-    return make_value_iterator<Policy>(std::begin(value), std::end(value), extra...);
-}
-
 template <typename InputType, typename OutputType> void implicitly_convertible() {
     struct set_flag {
         bool &flag;

tests/test_sequences_and_iterators.cpp

@@ -15,7 +15,6 @@
 
 #include <algorithm>
 #include <utility>
-#include <vector>
 
 #ifdef PYBIND11_HAS_OPTIONAL
 #include <optional>
@@ -38,29 +37,6 @@ bool operator==(const NonZeroIterator<std::pair<A, B>>& it, const NonZeroSentine
     return !(*it).first || !(*it).second;
 }
 
-class NonCopyableInt {
-public:
-    explicit NonCopyableInt(int value) : value_(value) {}
-    NonCopyableInt(const NonCopyableInt &) = delete;
-    NonCopyableInt(NonCopyableInt &&other) noexcept : value_(other.value_) {
-        other.value_ = -1;  // detect when an unwanted move occurs
-    }
-    NonCopyableInt &operator=(const NonCopyableInt &) = delete;
-    NonCopyableInt &operator=(NonCopyableInt &&other) noexcept {
-        value_ = other.value_;
-        other.value_ = -1;  // detect when an unwanted move occurs
-        return *this;
-    }
-    int get() const { return value_; }
-    void set(int value) { value_ = value; }
-    ~NonCopyableInt() = default;
-private:
-    int value_;
-};
-using NonCopyableIntPair = std::pair<NonCopyableInt, NonCopyableInt>;
-PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableInt>);
-PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableIntPair>);
-
 template <typename PythonType>
 py::list test_random_access_iterator(PythonType x) {
     if (x.size() < 5)
@@ -312,10 +288,6 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
         .def(
             "items",
             [](const StringMap &map) { return py::make_iterator(map.begin(), map.end()); },
-            py::keep_alive<0, 1>())
-        .def(
-            "values",
-            [](const StringMap &map) { return py::make_value_iterator(map.begin(), map.end()); },
             py::keep_alive<0, 1>());
 
     // test_generalized_iterators
@@ -334,38 +306,8 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
         .def("nonzero_keys", [](const IntPairs& s) {
             return py::make_key_iterator(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
         }, py::keep_alive<0, 1>())
-        .def("nonzero_values", [](const IntPairs& s) {
-            return py::make_value_iterator(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
-        }, py::keep_alive<0, 1>())
         ;
 
-    // test_iterater_referencing
-    py::class_<NonCopyableInt>(m, "NonCopyableInt")
-        .def(py::init<int>())
-        .def("set", &NonCopyableInt::set)
-        .def("__int__", &NonCopyableInt::get)
-        ;
-    py::class_<std::vector<NonCopyableInt>>(m, "VectorNonCopyableInt")
-        .def(py::init<>())
-        .def("append", [](std::vector<NonCopyableInt> &vec, int value) {
-            vec.emplace_back(value);
-        })
-        .def("__iter__", [](std::vector<NonCopyableInt> &vec) {
-            return py::make_iterator(vec.begin(), vec.end());
-        })
-        ;
-    py::class_<std::vector<NonCopyableIntPair>>(m, "VectorNonCopyableIntPair")
-        .def(py::init<>())
-        .def("append", [](std::vector<NonCopyableIntPair> &vec, const std::pair<int, int> &value) {
-            vec.emplace_back(NonCopyableInt(value.first), NonCopyableInt(value.second));
-        })
-        .def("keys", [](std::vector<NonCopyableIntPair> &vec) {
-            return py::make_key_iterator(vec.begin(), vec.end());
-        })
-        .def("values", [](std::vector<NonCopyableIntPair> &vec) {
-            return py::make_value_iterator(vec.begin(), vec.end());
-        })
-        ;
 
 #if 0
     // Obsolete: special data structure for exposing custom iterator types to python

tests/test_sequences_and_iterators.py

@@ -36,10 +36,6 @@ def test_generalized_iterators():
     assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero_keys()) == [1]
     assert list(m.IntPairs([(0, 3), (1, 2), (3, 4)]).nonzero_keys()) == []
 
-    assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).nonzero_values()) == [2, 4]
-    assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero_values()) == [2]
-    assert list(m.IntPairs([(0, 3), (1, 2), (3, 4)]).nonzero_values()) == []
-
     # __next__ must continue to raise StopIteration
     it = m.IntPairs([(0, 0)]).nonzero()
     for _ in range(3):
@@ -52,30 +48,6 @@ def test_generalized_iterators():
             next(it)
 
 
-def test_iterator_referencing():
-    """Test that iterators reference rather than copy their referents."""
-    vec = m.VectorNonCopyableInt()
-    vec.append(3)
-    vec.append(5)
-    assert [int(x) for x in vec] == [3, 5]
-    # Increment everything to make sure the referents can be mutated
-    for x in vec:
-        x.set(int(x) + 1)
-    assert [int(x) for x in vec] == [4, 6]
-
-    vec = m.VectorNonCopyableIntPair()
-    vec.append([3, 4])
-    vec.append([5, 7])
-    assert [int(x) for x in vec.keys()] == [3, 5]
-    assert [int(x) for x in vec.values()] == [4, 7]
-    for x in vec.keys():
-        x.set(int(x) + 1)
-    for x in vec.values():
-        x.set(int(x) + 10)
-    assert [int(x) for x in vec.keys()] == [4, 6]
-    assert [int(x) for x in vec.values()] == [14, 17]
-
-
 def test_sliceable():
     sliceable = m.Sliceable(100)
     assert sliceable[::] == (0, 100, 1)
@@ -179,7 +151,6 @@ def test_map_iterator():
         assert sm[k] == expected[k]
     for k, v in sm.items():
         assert v == expected[k]
-    assert list(sm.values()) == [expected[k] for k in sm]
 
     it = iter(m.StringMap({}))
     for _ in range(3):  # __next__ must continue to raise StopIteration