Skip to content

Commit

Permalink
make_iterator: accept args by value, not forwarding reference, to red…
Browse files Browse the repository at this point in the history
…uce dangling risk
  • Loading branch information
oremanj authored and wjakob committed Nov 14, 2024
1 parent 4d55fbb commit 7f4bae1
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 10 deletions.
6 changes: 3 additions & 3 deletions docs/api_extra.rst
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ include directive:
#include <nanobind/make_iterator.h>
.. cpp:function:: template <rv_policy Policy = rv_policy::automatic_reference, typename Iterator, typename... Extra> auto make_iterator(handle scope, const char * name, Iterator &&first, Iterator &&last, Extra &&...extra)
.. cpp:function:: template <rv_policy Policy = rv_policy::automatic_reference, typename Iterator, typename Sentinel, typename... Extra> auto make_iterator(handle scope, const char * name, Iterator first, Sentinel last, Extra &&...extra)

Create a Python iterator wrapping the C++ iterator represented by the range
``[first, last)``. The `Extra` parameter can be used to pass additional
Expand Down Expand Up @@ -619,7 +619,7 @@ include directive:
``first`` and ``last`` set to ``std::begin(value)`` and ``std::end(value)``,
respectively.

.. cpp:function:: template <rv_policy Policy = rv_policy::automatic_reference, typename Iterator, typename... Extra> iterator make_key_iterator(handle scope, const char * name, Iterator &&first, Iterator &&last, Extra &&...extra)
.. cpp:function:: template <rv_policy Policy = rv_policy::automatic_reference, typename Iterator, typename Sentinel, typename... Extra> iterator make_key_iterator(handle scope, const char * name, Iterator first, Sentinel last, Extra &&...extra)

:cpp:func:`make_iterator` specialization for C++ iterators that return
key-value pairs. `make_key_iterator` returns the first pair element to
Expand All @@ -630,7 +630,7 @@ include directive:
``(*first).first``.


.. cpp:function:: template <rv_policy Policy = rv_policy::automatic_reference, typename Iterator, typename... Extra> iterator make_value_iterator(handle scope, const char * name, Iterator &&first, Iterator &&last, Extra &&...extra)
.. cpp:function:: template <rv_policy Policy = rv_policy::automatic_reference, typename Iterator, typename Sentinel, typename... Extra> iterator make_value_iterator(handle scope, const char * name, Iterator first, Sentinel last, Extra &&...extra)

:cpp:func:`make_iterator` specialization for C++ iterators that return
key-value pairs. `make_value_iterator` returns the second pair element to
Expand Down
5 changes: 5 additions & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ Version TBD (unreleased)
documentation for more details, important caveats, and an example policy.
(PR `#767 <https://github.com/wjakob/nanobind/pull/767>`__)

- :cpp:func:`make_iterator` now accepts its iterator arguments by value,
rather than by forwarding reference, in order to eliminate the hazard
of storing a dangling C++ iterator reference in the returned Python
iterator object. (PR `#788 <https://github.com/wjakob/nanobind/pull/788>`__)

Version 2.2.0 (October 3, 2024)
-------------------------------

Expand Down
16 changes: 9 additions & 7 deletions include/nanobind/make_iterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ template <typename Iterator> struct iterator_value_access {
template <typename Access, rv_policy Policy, typename Iterator,
typename Sentinel, typename ValueType, typename... Extra>
typed<iterator, ValueType> make_iterator_impl(handle scope, const char *name,
Iterator &&first, Sentinel &&last,
Iterator first, Sentinel last,
Extra &&...extra) {
using State = iterator_state<Access, Policy, Iterator, Sentinel, ValueType, Extra...>;

Expand Down Expand Up @@ -103,8 +103,9 @@ template <rv_policy Policy = rv_policy::automatic_reference,
typename Iterator,
typename Sentinel,
typename ValueType = typename detail::iterator_access<Iterator>::result_type,
typename... Extra>
auto make_iterator(handle scope, const char *name, Iterator &&first, Sentinel &&last, Extra &&...extra) {
typename... Extra,
typename = decltype(std::declval<Iterator>() == std::declval<Sentinel>())>
auto make_iterator(handle scope, const char *name, Iterator first, Sentinel last, Extra &&...extra) {
return detail::make_iterator_impl<detail::iterator_access<Iterator>, Policy,
Iterator, Sentinel, ValueType, Extra...>(
scope, name, std::forward<Iterator>(first),
Expand All @@ -118,8 +119,8 @@ template <rv_policy Policy = rv_policy::automatic_reference, typename Iterator,
typename KeyType =
typename detail::iterator_key_access<Iterator>::result_type,
typename... Extra>
auto make_key_iterator(handle scope, const char *name, Iterator &&first,
Sentinel &&last, Extra &&...extra) {
auto make_key_iterator(handle scope, const char *name, Iterator first,
Sentinel last, Extra &&...extra) {
return detail::make_iterator_impl<detail::iterator_key_access<Iterator>,
Policy, Iterator, Sentinel, KeyType,
Extra...>(
Expand All @@ -134,7 +135,7 @@ template <rv_policy Policy = rv_policy::automatic_reference,
typename Sentinel,
typename ValueType = typename detail::iterator_value_access<Iterator>::result_type,
typename... Extra>
auto make_value_iterator(handle scope, const char *name, Iterator &&first, Sentinel &&last, Extra &&...extra) {
auto make_value_iterator(handle scope, const char *name, Iterator first, Sentinel last, Extra &&...extra) {
return detail::make_iterator_impl<detail::iterator_value_access<Iterator>,
Policy, Iterator, Sentinel, ValueType,
Extra...>(
Expand All @@ -145,7 +146,8 @@ auto make_value_iterator(handle scope, const char *name, Iterator &&first, Senti
/// Makes an iterator over values of a container supporting `std::begin()`/`std::end()`
template <rv_policy Policy = rv_policy::automatic_reference,
typename Type,
typename... Extra>
typename... Extra,
typename = decltype(std::begin(std::declval<Type&>()))>
auto make_iterator(handle scope, const char *name, Type &value, Extra &&...extra) {
return make_iterator<Policy>(scope, name, std::begin(value),
std::end(value),
Expand Down
15 changes: 15 additions & 0 deletions tests/test_make_iterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ NB_MODULE(test_make_iterator_ext, m) {
map.begin(),
map.end());
}, nb::keep_alive<0, 1>())
.def("items_l",
[](const StringMap &map) {
// Make sure iterators don't dangle even if passed as lvalue
auto begin = map.begin(), end = map.end();
return nb::make_iterator(nb::type<StringMap>(),
"item_iterator_l",
begin, end);
}, nb::keep_alive<0, 1>())
.def("values", [](const StringMap &map) {
return nb::make_value_iterator(nb::type<StringMap>(),
"value_iterator",
Expand Down Expand Up @@ -72,6 +80,13 @@ NB_MODULE(test_make_iterator_ext, m) {
map.begin(),
map.end());
}, nb::keep_alive<0, 1>())
.def("items_l",
[](const IdentityMap &map) {
auto begin = map.begin(), end = map.end();
return nb::make_iterator(nb::type<IdentityMap>(),
"item_iterator_l",
begin, end);
}, nb::keep_alive<0, 1>())
.def("values", [](const IdentityMap &map) {
return nb::make_value_iterator(nb::type<IdentityMap>(),
"value_iterator",
Expand Down
2 changes: 2 additions & 0 deletions tests/test_make_iterator.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def test03_items_iterator():
for d in data:
m = t.StringMap(d)
assert sorted(list(m.items())) == sorted(list(d.items()))
assert sorted(list(m.items_l())) == sorted(list(d.items()))


def test04_passthrough_iterator():
Expand All @@ -40,3 +41,4 @@ def test05_iterator_returning_temporary():
assert list(im) == list(range(10))
assert list(im.values()) == list(range(10))
assert list(im.items()) == list(zip(range(10), range(10)))
assert list(im.items_l()) == list(zip(range(10), range(10)))
4 changes: 4 additions & 0 deletions tests/test_make_iterator_ext.pyi.ref
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ class IdentityMap:

def items(self) -> Iterator[tuple[int, int]]: ...

def items_l(self) -> Iterator[tuple[int, int]]: ...

def values(self) -> Iterator[int]: ...

class StringMap:
Expand All @@ -22,6 +24,8 @@ class StringMap:

def items(self) -> Iterator[tuple[str, str]]: ...

def items_l(self) -> Iterator[tuple[str, str]]: ...

def values(self) -> Iterator[str]: ...

__all__: list = ['iterator_passthrough', 'StringMap', 'IdentityMap']
Expand Down

0 comments on commit 7f4bae1

Please sign in to comment.