From 2fa3fcfda5bb1aa1e8efc4d9cf90951e8055375e Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Wed, 22 Sep 2021 22:50:29 -0400 Subject: [PATCH] Revert "Add make_value_iterator (#3271)" This reverts commit ee0c5ee405e7a532410797687da28a20b89cd62b. --- docs/reference.rst | 3 - include/pybind11/pybind11.h | 118 +++++++------------------ tests/test_sequences_and_iterators.cpp | 58 ------------ tests/test_sequences_and_iterators.py | 29 ------ 4 files changed, 32 insertions(+), 176 deletions(-) diff --git a/docs/reference.rst b/docs/reference.rst index e64a03519..a678d41c8 100644 --- a/docs/reference.rst +++ b/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_`` diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index ac95b3a33..b8f5a6bae 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1955,52 +1955,25 @@ inline std::pair all_t return res; } -/* There are a large number of apparently unused template arguments because - * each combination requires a separate py::class_ registration. - */ -template +template 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 -struct iterator_access { - using result_type = decltype((*std::declval())); - // NOLINTNEXTLINE(readability-const-return-type) // PR #3263 - result_type operator()(Iterator &it) const { - return *it; - } -}; +PYBIND11_NAMESPACE_END(detail) -template -struct iterator_key_access { - using result_type = decltype(((*std::declval()).first)); - result_type operator()(Iterator &it) const { - return (*it).first; - } -}; - -template -struct iterator_value_access { - using result_type = decltype(((*std::declval()).second)); - result_type operator()(Iterator &it) const { - return (*it).second; - } -}; - -template ()), +#endif typename... Extra> -iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&... extra) { - using state = detail::iterator_state; - // 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; if (!detail::get_type_info(typeid(state), false)) { class_(handle(), "iterator", pybind11::module_local()) @@ -2014,7 +1987,7 @@ 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)..., Policy); } @@ -2022,55 +1995,35 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&... extra) { return cast(state{first, last, true}); } -PYBIND11_NAMESPACE_END(detail) - -/// Makes a python iterator from a first and past-the-end C++ InputIterator. -template ::result_type, - typename... Extra> -iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) { - return detail::make_iterator_impl< - detail::iterator_access, - Policy, - Iterator, - Sentinel, - ValueType, - Extra...>(first, last, std::forward(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 ::result_type, +#ifndef DOXYGEN_SHOULD_SKIP_THIS // Issue in breathe 4.26.1 + typename KeyType = decltype((*std::declval()).first), +#endif typename... Extra> iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) { - return detail::make_iterator_impl< - detail::iterator_key_access, - Policy, - Iterator, - Sentinel, - KeyType, - Extra...>(first, last, std::forward(extra)...); -} + using state = detail::iterator_state; -/// Makes a python iterator over the values (`.second`) of a iterator over pairs from a -/// first and past-the-end InputIterator. -template ::result_type, - typename... Extra> -iterator make_value_iterator(Iterator first, Sentinel last, Extra &&...extra) { - return detail::make_iterator_impl< - detail::iterator_value_access, - Policy, Iterator, - Sentinel, - ValueType, - Extra...>(first, last, std::forward(extra)...); + if (!detail::get_type_info(typeid(state), false)) { + class_(handle(), "iterator", pybind11::module_local()) + .def("__iter__", [](state &s) -> state& { return s; }) + .def("__next__", [](state &s) -> detail::remove_cv_t { + 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)..., 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 (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 iterator make_value_iterator(Type &value, Extra&&... extra) { - return make_value_iterator(std::begin(value), std::end(value), extra...); -} - template void implicitly_convertible() { struct set_flag { bool &flag; diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp index 72d96cb44..f4a78ae99 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -15,7 +15,6 @@ #include #include -#include #ifdef PYBIND11_HAS_OPTIONAL #include @@ -38,29 +37,6 @@ bool operator==(const NonZeroIterator>& 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; -PYBIND11_MAKE_OPAQUE(std::vector); -PYBIND11_MAKE_OPAQUE(std::vector); - template 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>(s.begin()), NonZeroSentinel()); }, py::keep_alive<0, 1>()) - .def("nonzero_values", [](const IntPairs& s) { - return py::make_value_iterator(NonZeroIterator>(s.begin()), NonZeroSentinel()); - }, py::keep_alive<0, 1>()) ; - // test_iterater_referencing - py::class_(m, "NonCopyableInt") - .def(py::init()) - .def("set", &NonCopyableInt::set) - .def("__int__", &NonCopyableInt::get) - ; - py::class_>(m, "VectorNonCopyableInt") - .def(py::init<>()) - .def("append", [](std::vector &vec, int value) { - vec.emplace_back(value); - }) - .def("__iter__", [](std::vector &vec) { - return py::make_iterator(vec.begin(), vec.end()); - }) - ; - py::class_>(m, "VectorNonCopyableIntPair") - .def(py::init<>()) - .def("append", [](std::vector &vec, const std::pair &value) { - vec.emplace_back(NonCopyableInt(value.first), NonCopyableInt(value.second)); - }) - .def("keys", [](std::vector &vec) { - return py::make_key_iterator(vec.begin(), vec.end()); - }) - .def("values", [](std::vector &vec) { - return py::make_value_iterator(vec.begin(), vec.end()); - }) - ; #if 0 // Obsolete: special data structure for exposing custom iterator types to python diff --git a/tests/test_sequences_and_iterators.py b/tests/test_sequences_and_iterators.py index 79689391a..44069fdd1 100644 --- a/tests/test_sequences_and_iterators.py +++ b/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