Skip to content
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

[FEAT] docs: keep_alive should have more nuanced explanation for constructors? #2978

Closed
EricCousineau-TRI opened this issue Apr 27, 2021 · 2 comments
Assignees
Labels
docs Docs or GitHub info enhancement

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Apr 27, 2021

At present, pybind11 docs for keep alive are as follows:
https://pybind11.readthedocs.io/en/stable/advanced/functions.html?highlight=keep_alive#keep-alive
https://github.com/pybind/pybind11/blob/v2.6.2/docs/advanced/functions.rst#keep-alive

Argument indices start at one, while zero refers to the return value. For methods, index 1 refers to the implicit this pointer, while regular arguments begin at index 2.

However, it doesn't cover cases like constructors, either via vanilla py::init(), or factory-style constructors, as indicated here:
https://pybind11.readthedocs.io/en/stable/advanced/classes.html#custom-constructors

I think we (Drake devs) operate under the assumption that a constructor functions as a method, and thus keep_alive<1, 2>() would be something like "Keep alive: self keeps first arg alive", but I'm not sure upon reinspection.

Motivation: RobotLocomotion/drake#14925 (review)

\cc @hongkai-dai

@EricCousineau-TRI EricCousineau-TRI added enhancement docs Docs or GitHub info labels Apr 27, 2021
@EricCousineau-TRI EricCousineau-TRI self-assigned this Apr 27, 2021
@EricCousineau-TRI EricCousineau-TRI changed the title [FEAT] docs: keep_alive isn't sufficiently explained? (e.g. factory methods for constructors) [FEAT] docs: keep_alive should have more nuanced explanation for constructors? Apr 27, 2021
@EricCousineau-TRI
Copy link
Collaborator Author

Shown in unittests:

py::class_<Parent>(m, "Parent")
.def(py::init<>())
.def(py::init([](Child *) { return new Parent(); }), py::keep_alive<1, 2>())
.def("addChild", &Parent::addChild)
.def("addChildKeepAlive", &Parent::addChild, py::keep_alive<1, 2>())
.def("returnChild", &Parent::returnChild)
.def("returnChildKeepAlive", &Parent::returnChild, py::keep_alive<1, 0>())
.def("returnNullChildKeepAliveChild", &Parent::returnNullChild, py::keep_alive<1, 0>())
.def("returnNullChildKeepAliveParent", &Parent::returnNullChild, py::keep_alive<0, 1>());

Add'ly substantiated: master...EricCousineau-TRI:issue-2978-wip

"""
Output, Ubuntu 18.04, using apt packages:
$ cd pybind11
$ mkdir build && cd build
$ cmake .. -DPYTHNO_EXECUTABLE=$(which python3)
$ make -j2 pytest
...
../../tests/test_keep_alive.py --- modulename: test_keep_alive, funcname: test_stuff
test_keep_alive.py(27): other = m.ExampleOther()
test_keep_alive.py(28): x = "a"
test_keep_alive.py(30): out = m.example_free_func(other)
keep_alive_impl( <ExampleReturn> , <ExampleOther> )
test_keep_alive.py(31): assert isinstance(out, m.ExampleReturn)
test_keep_alive.py(34): obj = m.ExampleSelf(other)
keep_alive_impl( <ExampleSelf> , <ExampleOther> )
test_keep_alive.py(36): m.ExampleSelf(other, str())
keep_alive_impl( <ExampleSelf> , <ExampleOther> )
test_keep_alive.py(37): m.ExampleSelf(other, int())
keep_alive_impl( None , <ExampleOther> )
test_keep_alive.py(39): out2 = obj.example_method(other)
keep_alive_impl( <ExampleSelf> , <ExampleOther> )
test_keep_alive.py(40): assert isinstance(out2, m.ExampleReturn)
test_keep_alive.py(42): out3 = obj.example_static_method(other)
keep_alive_impl( <ExampleReturn> , <ExampleOther> )
test_keep_alive.py(43): assert isinstance(out3, m.ExampleReturn)
"""

@EricCousineau-TRI
Copy link
Collaborator Author

Also, docs 🤦

For consistency, the argument indexing is identical for constructors. Index 1 still refers to the implicit this pointer, i.e. the object which is being constructed. *Index 0 refers to the return type which is presumed to be void when a constructor is viewed like a function. *

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Docs or GitHub info enhancement
Projects
None yet
Development

No branches or pull requests

1 participant