From ee0c5ee405e7a532410797687da28a20b89cd62b Mon Sep 17 00:00:00 2001 From: Bruce Merry <1963944+bmerry@users.noreply.github.com> Date: Tue, 21 Sep 2021 19:37:19 +0200 Subject: [PATCH] Add make_value_iterator (#3271) * Add make_value_iterator This is the counterpart to make_key_iterator, and will allow implementing a `value` method in `bind_map` (although doing so is left for a subsequent PR). I made a few design changes to reduce copy-and-paste boilerplate. Previously detail::iterator_state had a boolean template parameter to indicate whether it was being used for make_iterator or make_key_iterator. I replaced the boolean with a class that determines how to dereference the iterator. This allows for a generic implementation of `__next__`. I also added the ValueType and Extra... parameters to the iterator_state template args, because I think it was a bug that they were missing: if make_iterator is called twice with different values of these, only the first set has effect (because the state class is only registered once). There is still a potential issue in that the *values* of the extra arguments are latched on the first call, but since most policies are empty classes this should be even less common. * Add some remove_cv_t to appease clang-tidy * Make iterator_access and friends take reference For some reason I'd accidentally made it take a const value, which caused some issues with third-party packages. * Another attempt to remove remove_cv_t from iterators Some of the return types were const (non-reference) types because of the pecularities of decltype: `decltype((*it).first)` is the *declared* type of the member of the pair, rather than the type of the expression. So if the reference type of the iterator is `pair &`, then the decltype is `const int`. Wrapping an extra set of parentheses to form `decltype(((*it).first))` would instead give `const int &`. This means that the existing make_key_iterator actually returns by value from `__next__`, rather than by reference. Since for mapping types, keys are always const, this probably hasn't been noticed, but it will affect make_value_iterator if the Python code tries to mutate the returned objects. I've changed things to use double parentheses so that make_iterator, make_key_iterator and make_value_iterator should now all return the reference type of the iterator. I'll still need to add a test for that; for now I'm just checking whether I can keep Clang-Tidy happy. * Add back some NOLINTNEXTLINE to appease Clang-Tidy This is favoured over using remove_cv_t because in some cases a const value return type is deliberate (particularly for Eigen). * Add a unit test for iterator referencing Ensure that make_iterator, make_key_iterator and make_value_iterator return references to the container elements, rather than copies. The test for make_key_iterator fails to compile on master, which gives me confidence that this branch has fixed it. * Make the iterator_access etc operator() const I'm actually a little surprised it compiled at all given that the operator() is called on a temporary, but I don't claim to fully understand all the different value types in C++11. * Attempt to work around compiler bugs https://godbolt.org/ shows an example where ICC gets the wrong result for a decltype used as the default for a template argument, and CI also showed problems with PGI. This is a shot in the dark to see if it fixes things. * Make a test constructor explicit (Clang-Tidy) * Fix unit test on GCC 4.8.5 It seems to require the arguments to the std::pair constructor to be implicitly convertible to the types in the pair, rather than just requiring is_constructible. * Remove DOXYGEN_SHOULD_SKIP_THIS guards Now that a complex decltype expression has been replaced by a simpler nested type, I'm hoping Doxygen will be able to build it without issues. * Add comment to explain iterator_state template params --- 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, 176 insertions(+), 32 deletions(-) diff --git a/docs/reference.rst b/docs/reference.rst index a678d41c8..e64a03519 100644 --- a/docs/reference.rst +++ b/docs/reference.rst @@ -63,6 +63,9 @@ 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 b8f5a6bae..ac95b3a33 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1955,25 +1955,52 @@ inline std::pair all_t return res; } -template +/* There are a large number of apparently unused template arguments because + * each combination requires a separate py::class_ registration. + */ +template struct iterator_state { Iterator it; Sentinel end; bool first_or_done; }; -PYBIND11_NAMESPACE_END(detail) +// 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; + } +}; -/// Makes a python iterator from a first and past-the-end C++ InputIterator. -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 ValueType, typename... Extra> -iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) { - using state = detail::iterator_state; +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 if (!detail::get_type_info(typeid(state), false)) { class_(handle(), "iterator", pybind11::module_local()) @@ -1987,7 +2014,7 @@ iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) { s.first_or_done = true; throw stop_iteration(); } - return *s.it; + return Access()(s.it); // NOLINTNEXTLINE(readability-const-return-type) // PR #3263 }, std::forward(extra)..., Policy); } @@ -1995,35 +2022,55 @@ iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) { return cast(state{first, last, true}); } -/// Makes an python iterator over the keys (`.first`) of a iterator over pairs from a +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 /// first and past-the-end InputIterator. template ()).first), -#endif + typename KeyType = typename detail::iterator_key_access::result_type, typename... Extra> iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) { - using state = detail::iterator_state; + return detail::make_iterator_impl< + detail::iterator_key_access, + Policy, + Iterator, + Sentinel, + KeyType, + 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 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)...); } /// Makes an iterator over values of an stl container or other container supporting @@ -2040,6 +2087,13 @@ 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 b07fd197a..66d647262 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -15,6 +15,7 @@ #include #include +#include template class NonZeroIterator { @@ -32,6 +33,29 @@ 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) @@ -271,6 +295,10 @@ 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 @@ -289,8 +317,38 @@ 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 a868c542c..2c73eff29 100644 --- a/tests/test_sequences_and_iterators.py +++ b/tests/test_sequences_and_iterators.py @@ -25,6 +25,10 @@ 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): @@ -37,6 +41,30 @@ 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) @@ -140,6 +168,7 @@ 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