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

Enable casting unique_ptr arguments and ownership transfer, #1237

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

EricCousineau-TRI
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI commented Jan 6, 2018

This PR enables passing unique_ptr back and forth between C++.
This is a refactored, simplified, and more manageable chunk of #1146 (supporting #1132).

This relates #971, #1226.

I'm moving forward with this because we have found this feature useful in Drake, and have a potential mechanism to even allow this to be REPL-friendly. (This change does require minimal changes in existing code, which is preferable to switching to shared_ptr and never being able to release a pointer from its clutches.)
https://github.com/RobotLocomotion/drake/blob/f6f23c5/bindings/pydrake/systems/framework_py.cc#L148
https://github.com/EricCousineau-TRI/pybind11/blob/25b08a4/tests/test_smart_ptr.py#L319

Additional motivation:
Google's clif can handle unique_ptrs as arguments, so it should be possible :)
https://github.com/google/clif/blob/master/clif/python/README.md#pointers-references-and-object-ownership

\cc @torokati44

@EricCousineau-TRI EricCousineau-TRI changed the title Feature/unique ptr arg pr1 Pave way for unique_ptr arguments Step 1: Prepare casters, add minor doc change Jan 6, 2018
@EricCousineau-TRI EricCousineau-TRI changed the title Pave way for unique_ptr arguments Step 1: Prepare casters, add minor doc change Step 1 for unique_ptr arguments: Prepare casters, add minor doc change Jan 6, 2018
@EricCousineau-TRI EricCousineau-TRI changed the title Step 1 for unique_ptr arguments: Prepare casters, add minor doc change Enable casting unique_ptr arguments and ownership transfer, Jan 7, 2018
@EricCousineau-TRI
Copy link
Collaborator Author

After the first portion of this PR, I realized that it wouldn't be too much more to enable unique_ptr as arguments.

Please note that I have curated each of the commits to make sure that each commit makes sense by itself.

This also includes a commit to fix #1238, because I encountered this issue while developing this branch.

@EricCousineau-TRI EricCousineau-TRI force-pushed the feature/unique_ptr_arg_pr1 branch 5 times, most recently from d1c1c2a to 966c333 Compare January 7, 2018 02:51
@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Jan 7, 2018

Hmm... CI failures seem possibly unrelated to this PR?

On Travis CI, there are errors where the assertion errors differ by a space.
On AppVeyor, there is a warning about using if constexpr (which I'd really prefer not to, as it's C++17), that I guess is then escalated to an error? (I can patch this with a temporary, but just wondering if this warning should be suppressed?)

May I ask if there are any ideas on the Travis CI NumPy stuff?

EDIT: Looks like Jason handled it here: #1246

@EricCousineau-TRI
Copy link
Collaborator Author

@jagerman Can I ask if you've had a chance to take a peek at this PR? Might you have any suggestions at a design level that might make this fit into a future pybind release?

@kunitoki
Copy link

This would be awesome, any chance to get this addressed ?

@rwgk
Copy link
Collaborator

rwgk commented Feb 19, 2024

This would be awesome, any chance to get this addressed ?

https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst

The branch exists for ~3 years now (and is used in Google production code since then). The last noteworthy change was in the summer of 2021. I'm keeping it up-to-date with master, and it is fully compatible with master (see the README).

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

Successfully merging this pull request may close these issues.

3 participants