Skip to content

Commit

Permalink
fix: Revert pfect args make iterator (#4234)
Browse files Browse the repository at this point in the history
* Revert "chore: perfectly forward all make_iterator args (#3980)"

This reverts commit 8da58da.

* Redo unrelated optimization in commit

* Add tests for ambiguous overloads
  • Loading branch information
Skylion007 authored Oct 21, 2022
1 parent 91cfb77 commit 17c1e27
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 14 deletions.
22 changes: 8 additions & 14 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -2333,7 +2333,7 @@ template <typename Access,
typename Sentinel,
typename ValueType,
typename... Extra>
iterator make_iterator_impl(Iterator &&first, Sentinel &&last, Extra &&...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

Expand All @@ -2359,7 +2359,7 @@ iterator make_iterator_impl(Iterator &&first, Sentinel &&last, Extra &&...extra)
Policy);
}

return cast(state{std::forward<Iterator>(first), std::forward<Sentinel>(last), true});
return cast(state{first, last, true});
}

PYBIND11_NAMESPACE_END(detail)
Expand All @@ -2370,15 +2370,13 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
typename Sentinel,
typename ValueType = typename detail::iterator_access<Iterator>::result_type,
typename... Extra>
iterator make_iterator(Iterator &&first, Sentinel &&last, Extra &&...extra) {
iterator make_iterator(Iterator first, Sentinel last, Extra &&...extra) {
return detail::make_iterator_impl<detail::iterator_access<Iterator>,
Policy,
Iterator,
Sentinel,
ValueType,
Extra...>(std::forward<Iterator>(first),
std::forward<Sentinel>(last),
std::forward<Extra>(extra)...);
Extra...>(first, last, std::forward<Extra>(extra)...);
}

/// Makes a python iterator over the keys (`.first`) of a iterator over pairs from a
Expand All @@ -2388,15 +2386,13 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
typename Sentinel,
typename KeyType = typename detail::iterator_key_access<Iterator>::result_type,
typename... Extra>
iterator make_key_iterator(Iterator &&first, Sentinel &&last, Extra &&...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...>(std::forward<Iterator>(first),
std::forward<Sentinel>(last),
std::forward<Extra>(extra)...);
Extra...>(first, last, std::forward<Extra>(extra)...);
}

/// Makes a python iterator over the values (`.second`) of a iterator over pairs from a
Expand All @@ -2406,15 +2402,13 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
typename Sentinel,
typename ValueType = typename detail::iterator_value_access<Iterator>::result_type,
typename... Extra>
iterator make_value_iterator(Iterator &&first, Sentinel &&last, Extra &&...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...>(std::forward<Iterator>(first),
std::forward<Sentinel>(last),
std::forward<Extra>(extra)...);
Extra...>(first, last, std::forward<Extra>(extra)...);
}

/// Makes an iterator over values of an stl container or other container supporting
Expand Down
19 changes: 19 additions & 0 deletions tests/test_sequences_and_iterators.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -559,4 +559,23 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
[]() { return py::make_iterator<py::return_value_policy::copy>(list); });
m.def("make_iterator_2",
[]() { return py::make_iterator<py::return_value_policy::automatic>(list); });

// test_iterator on c arrays
// #4100: ensure lvalue required as increment operand
class CArrayHolder {
public:
CArrayHolder(double x, double y, double z) {
values[0] = x;
values[1] = y;
values[2] = z;
};
double values[3];
};

py::class_<CArrayHolder>(m, "CArrayHolder")
.def(py::init<double, double, double>())
.def(
"__iter__",
[](const CArrayHolder &v) { return py::make_iterator(v.values, v.values + 3); },
py::keep_alive<0, 1>());
}
8 changes: 8 additions & 0 deletions tests/test_sequences_and_iterators.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,11 @@ def test_iterator_rvp():
assert list(m.make_iterator_1()) == [1, 2, 3]
assert list(m.make_iterator_2()) == [1, 2, 3]
assert not isinstance(m.make_iterator_1(), type(m.make_iterator_2()))


def test_carray_iterator():
"""#4100: Check for proper iterator overload with C-Arrays"""
args_gt = list(float(i) for i in range(3))
arr_h = m.CArrayHolder(*args_gt)
args = list(arr_h)
assert args_gt == args

0 comments on commit 17c1e27

Please sign in to comment.