feature: Support move-only iterators in py::make_*iterator (#4834)

* feature: Support move-only iterators in `py::make_*iterator` * fix: Missing static assertion message * fixup: Missing `explicit` in single argument constructors * fix: Simplify tests: make existing iterator move-only * fix: Missing `noexcept`
pybind · Sep 7, 2023 · c836059 · c836059
1 parent 4a2f7e4
commit c836059
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 4 deletions.
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -2434,7 +2434,7 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) {
                 Policy);
     }
 
-    return cast(state{first, last, true});
+    return cast(state{std::forward<Iterator>(first), std::forward<Sentinel>(last), true});
 }
 
 PYBIND11_NAMESPACE_END(detail)
@@ -2451,7 +2451,9 @@ iterator make_iterator(Iterator first, Sentinel last, Extra &&...extra) {
                                       Iterator,
                                       Sentinel,
                                       ValueType,
-                                      Extra...>(first, last, std::forward<Extra>(extra)...);
+                                      Extra...>(std::forward<Iterator>(first),
+                                                std::forward<Sentinel>(last),
+                                                std::forward<Extra>(extra)...);
 }
 
 /// Makes a python iterator over the keys (`.first`) of a iterator over pairs from a
@@ -2467,7 +2469,9 @@ iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) {
                                       Iterator,
                                       Sentinel,
                                       KeyType,
-                                      Extra...>(first, last, std::forward<Extra>(extra)...);
+                                      Extra...>(std::forward<Iterator>(first),
+                                                std::forward<Sentinel>(last),
+                                                std::forward<Extra>(extra)...);
 }
 
 /// Makes a python iterator over the values (`.second`) of a iterator over pairs from a
@@ -2483,7 +2487,9 @@ iterator make_value_iterator(Iterator first, Sentinel last, Extra &&...extra) {
                                       Iterator,
                                       Sentinel,
                                       ValueType,
-                                      Extra...>(first, last, std::forward<Extra>(extra)...);
+                                      Extra...>(std::forward<Iterator>(first),
+                                                std::forward<Sentinel>(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
@@ -28,6 +28,13 @@ class NonZeroIterator {
 
 public:
     explicit NonZeroIterator(const T *ptr) : ptr_(ptr) {}
+
+    // Make the iterator non-copyable and movable
+    NonZeroIterator(const NonZeroIterator &) = delete;
+    NonZeroIterator(NonZeroIterator &&) noexcept = default;
+    NonZeroIterator &operator=(const NonZeroIterator &) = delete;
+    NonZeroIterator &operator=(NonZeroIterator &&) noexcept = default;
+
     const T &operator*() const { return *ptr_; }
     NonZeroIterator &operator++() {
         ++ptr_;
@@ -78,6 +85,7 @@ class NonCopyableInt {
     int value_;
 };
 using NonCopyableIntPair = std::pair<NonCopyableInt, NonCopyableInt>;
+
 PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableInt>);
 PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableIntPair>);
 
@@ -375,6 +383,17 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
     private:
         std::vector<std::pair<int, int>> data_;
     };
+
+    {
+        // #4383 : Make sure `py::make_*iterator` functions work with move-only iterators
+        using iterator_t = NonZeroIterator<std::pair<int, int>>;
+
+        static_assert(std::is_move_assignable<iterator_t>::value, "");
+        static_assert(std::is_move_constructible<iterator_t>::value, "");
+        static_assert(!std::is_copy_assignable<iterator_t>::value, "");
+        static_assert(!std::is_copy_constructible<iterator_t>::value, "");
+    }
+
     py::class_<IntPairs>(m, "IntPairs")
         .def(py::init<std::vector<std::pair<int, int>>>())
         .def(