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..16535f195 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1955,25 +1955,54 @@ 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. C++ needs the extra parens in decltype +// to enforce an lvalue. The & after Iterator is required for MSVC < 16.9. SFINAE cannot be +// reused for result_type due to bugs in ICC, NVCC, and PGI compilers. See PR #3293. +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 ()).first)) > +struct iterator_key_access { + using result_type = decltype(((*std::declval()).first)); + result_type operator()(Iterator &it) const { + return (*it).first; + } +}; + +template ()).second))> +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 +2016,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 +2024,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 +2089,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 16f8f5b23..9de69338b 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -15,6 +15,7 @@ #include #include +#include #ifdef PYBIND11_HAS_OPTIONAL #include @@ -37,6 +38,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) @@ -288,6 +312,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 @@ -308,19 +336,62 @@ 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 single-argument make_iterator .def("simple_iterator", [](IntPairs& self) { return py::make_iterator(self); }, py::keep_alive<0, 1>()) .def("simple_keys", [](IntPairs& self) { return py::make_key_iterator(self); }, py::keep_alive<0, 1>()) + .def("simple_values", [](IntPairs& self) { + return py::make_value_iterator(self); + }, py::keep_alive<0, 1>()) - // test iterator with keep_alive (doesn't work so not used at runtime, but tests compile) - .def("make_iterator_keep_alive", [](IntPairs& self) { - return py::make_iterator(self, py::keep_alive<0, 1>()); + // Test iterator with an Extra (doesn't do anything useful, so not used + // at runtime, but tests need to be able to compile with the correct + // overload. See PR #3293. + .def("_make_iterator_extras", [](IntPairs& self) { + return py::make_iterator(self, py::call_guard()); + }, py::keep_alive<0, 1>()) + .def("_make_key_extras", [](IntPairs& self) { + return py::make_key_iterator(self, py::call_guard()); + }, py::keep_alive<0, 1>()) + .def("_make_value_extras", [](IntPairs& self) { + return py::make_value_iterator(self, py::call_guard()); }, 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 902f4914c..38e2ab5b7 100644 --- a/tests/test_sequences_and_iterators.py +++ b/tests/test_sequences_and_iterators.py @@ -36,6 +36,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): @@ -55,6 +59,31 @@ def test_generalized_iterators_simple(): (0, 5), ] assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).simple_keys()) == [1, 3, 0] + assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).simple_values()) == [2, 4, 5] + + +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(): @@ -160,6 +189,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