Skip to content

feat: reapply fixed version of #3271 #3293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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_``
Expand Down
120 changes: 88 additions & 32 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1955,25 +1955,54 @@ inline std::pair<decltype(internals::registered_types_py)::iterator, bool> all_t
return res;
}

template <typename Iterator, typename Sentinel, bool KeyIterator, return_value_policy Policy>
/* 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>
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 <typename Iterator, typename SFINAE = decltype((*std::declval<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;
}
};

/// Makes a python iterator from a first and past-the-end C++ InputIterator.
template <return_value_policy Policy = return_value_policy::reference_internal,
template <typename Iterator, typename SFINAE = decltype(((*std::declval<Iterator &>()).first)) >
struct iterator_key_access {
using result_type = decltype(((*std::declval<Iterator &>()).first));
result_type operator()(Iterator &it) const {
return (*it).first;
}
};

template <typename Iterator, typename SFINAE = decltype(((*std::declval<Iterator &>()).second))>
struct iterator_value_access {
using result_type = decltype(((*std::declval<Iterator &>()).second));
result_type operator()(Iterator &it) const {
return (*it).second;
}
};

template <typename Access,
return_value_policy Policy,
typename Iterator,
typename Sentinel,
#ifndef DOXYGEN_SHOULD_SKIP_THIS // Issue in breathe 4.26.1
typename ValueType = decltype(*std::declval<Iterator>()),
#endif
typename ValueType,
typename... Extra>
iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
using state = detail::iterator_state<Iterator, Sentinel, false, Policy>;
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

if (!detail::get_type_info(typeid(state), false)) {
class_<state>(handle(), "iterator", pybind11::module_local())
Expand All @@ -1987,43 +2016,63 @@ 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>(extra)..., Policy);
}

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 <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
/// first and past-the-end InputIterator.
template <return_value_policy Policy = return_value_policy::reference_internal,
typename Iterator,
typename Sentinel,
#ifndef DOXYGEN_SHOULD_SKIP_THIS // Issue in breathe 4.26.1
typename KeyType = decltype((*std::declval<Iterator>()).first),
#endif
typename KeyType = typename detail::iterator_key_access<Iterator>::result_type,
typename... Extra>
iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) {
using state = detail::iterator_state<Iterator, Sentinel, true, Policy>;

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 detail::make_iterator_impl<
detail::iterator_key_access<Iterator>,
Policy,
Iterator,
Sentinel,
KeyType,
Extra...>(first, last, std::forward<Extra>(extra)...);
}

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 <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)...);
}

/// Makes an iterator over values of an stl container or other container supporting
Expand All @@ -2040,6 +2089,13 @@ 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;
Expand Down
77 changes: 74 additions & 3 deletions tests/test_sequences_and_iterators.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include <algorithm>
#include <utility>
#include <vector>

#ifdef PYBIND11_HAS_OPTIONAL
#include <optional>
Expand All @@ -37,6 +38,29 @@ 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)
Expand Down Expand Up @@ -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
Expand All @@ -308,19 +336,62 @@ 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 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<int>());
}, py::keep_alive<0, 1>())
.def("_make_key_extras", [](IntPairs& self) {
return py::make_key_iterator(self, py::call_guard<int>());
}, py::keep_alive<0, 1>())
.def("_make_value_extras", [](IntPairs& self) {
return py::make_value_iterator(self, py::call_guard<int>());
}, 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
Expand Down
30 changes: 30 additions & 0 deletions tests/test_sequences_and_iterators.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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():
Expand Down Expand Up @@ -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
Expand Down