-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
WIP: Support ownership transfer between C++ and Python with shared_ptr<T>
and unique_ptr<T>
for pure C++ instances and single-inheritance instances
#1146
Conversation
shared_ptr<T>
and unique_ptr<T>
shared_ptr<T>
and unique_ptr<T>
for pure C++ instances and single-inheritance instances
I don't think it's going to be feasible for us to even consider a commit of this scale to support moving |
I understand on the commit scale. I can certainly break this into smaller components to take this out of a WIP stage. (I can also collapse the commits - those were me just prototyping small tests as I was learning some of the internals of For moving This also includes preventing Python-subclass aliasing that would arise from passing |
1d48c78
to
9e7b022
Compare
I've squashed the commits in this branch. All of the granular experimental history is in The workflow for derived-type aliasing, specifically with Still on my TODO is the |
I don't think squashing everything into a single commit addresses the problem, which is really the sheer size of the diff. I'll try to take a look in the next 1-2 weeks regarding the functional aspects (sorry for the delay, I have an unrelated deadline ATM). |
Sorry, I didn't mean to imply that squashing was the only problem :) Thanks! |
One thing that I'm missing is a concise overview of what this feature is all about. Can I suggest that you write up a summary of a few paragraphs aimed at someone who has some pybind experience, but not a core developer that describes the motivation, feature, and simple implementation. (The reason I ask for a experienced-but-not-core-developer target is that this isn't just for evaluation of the PR: it would also form the basis of the |
Most certainly! Thank you for your patience and for considering this! |
I've added a quick draft of the main features of what this PR could introduce at a user-facing level. If you have time, please let me know if you have any comments! |
I have updated the documentation, and included details on the holder stuff for ownership transfer as well: EDIT: If you would like to see the overview in a different form, please let me know! |
@ppt-adsk mentioned that they have ran into a similar issue, at least for inheritance with That being said, can I ask if there is any way that I could carve up the overview docs or this PR to make it easier to review? |
a1e9679
to
f7cf29c
Compare
3bfa9ea
to
a1a12b6
Compare
01d6742
to
1930e66
Compare
d7162d8
to
2df39b2
Compare
…` and `unique_ptr<T>` for pure C++ instances and single-inheritance instances
2df39b2
to
93d99ac
Compare
I'm going to decree this PR as stale. I've updated the overview to indicate where to look for more up-to-date discussions. |
UPDATE (2020-12-28): Decreed as stale.
For a more up-to-date and working implementation, see https://github.com/RobotLocomotion/pybind11/blob/drake/README_DRAKE.md (permalink)
This addresses #1132 and #1145 (and accommodates #1138 / incorporates #1139).
All unittests pass on my system (Python 2.7, release + debug).
Todo
detail::type_info
anddetail::instance
pybind11::wrapper<Base>
master
, make PR tomaster
.