fix: Revert pfect args make iterator (pybind#4234)

* Revert "chore: perfectly forward all make_iterator args (pybind#3980)" This reverts commit 8da58da. * Redo unrelated optimization in commit * Add tests for ambiguous overloads
henryiii · Oct 21, 2022 · 305c471 · 305c471
1 parent eaa5f7b
commit 305c471
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 14 deletions.
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -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
 
@@ -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)
@@ -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
@@ -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
@@ -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

diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp
@@ -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>());
 }
diff --git a/tests/test_sequences_and_iterators.py b/tests/test_sequences_and_iterators.py
@@ -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