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

Smart Pointers for non custom types #1985

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

Conversation

flippmoke
Copy link
Contributor

Added a way for non custom types to take advantage of smart pointers which hopefully solves #787.

@flippmoke
Copy link
Contributor Author

Some notes:

I know this change should update documentation and I am willing to do so, but would love to get feedback on implementation before updating docs.

For backwards compatibility with individuals using holder_helper I could create other new structs as helpers here, but it felt strange as I tried to design the interface. This was especially true due to the const vs non const get that was implemented.

I did consider making the different versions of copyable_holder_caster using SFINAE, but I couldn't seem to get it work properly. If someone thinks that would be a better approach I can attempt to do it.

I didn't add as many tests for situations that are a bit odd and will add some tests for them again if this approach is useful. One example would be doing something like the following which would create the possibility that both methods are created I think -- I am not sure how SFINAE would work here.

py::bind_vector<std::vector<Object>, std::shared_ptr<std::vector<Object>>>(m, "ObjectVec");

@flippmoke
Copy link
Contributor Author

I just realized my implementation has some issues with the correct return_value_policy and handling correctly the lifetime of objects. I am going to attempt to correct this, so still a WIP.

@flippmoke
Copy link
Contributor Author

I think this fixed the return value policy but I think some review here if the return_value_policy_override is going to be unexpected as I think it might be in some situations. For example:

If you have a custom binding for a class MyObject, but if it does not use a shared_ptr for internal storage, a return value of std::shared_ptr<MyObject> would have its value moved out (I think). This might be unexpected for some users.

@flippmoke
Copy link
Contributor Author

@wjakob This is ready for a full review, but if this is is this something you don't see the library implementing let me know and I can close the PR and approach this in my work in some other way. Thanks!

@DmitriGoloubentsev
Copy link

@flippmoke Did you find a workaround for this? I'm surprised this PR didn't get deserved attention.

Using shared_ptr<std::vector> is pretty common, but pybind fails for me on this.

@rwgk
Copy link
Collaborator

rwgk commented Aug 24, 2024

Oh, interesting, I never noticed this PR before. I need to look closer.

But in the meantime, for years, I worked on this, and it's used by Google since spring 2021:

https://github.com/pybind/pybind11/tree/smart_holder

Just use that. It has everything that the master branch has. All you need to do is git clone from the smart_holder branch and then use py::classh instead of py::class_ (and only when it actually matters for your use cases).

For the past couple months I've been working on getting it ready for merging into master. At the moment I'm reviewing the handling of const unique_ptr<T> &, I didn't give that enough attention before; but that's only a pretty obscure and small part of the unique_ptr/shared_ptr functionality.

@DmitriGoloubentsev
Copy link

@rwgk Do you have plans to merge it in master?

@rwgk
Copy link
Collaborator

rwgk commented Aug 27, 2024

@rwgk Do you have plans to merge it in master?

#5339, created yesterday. Waiting for feedback.

@rwgk
Copy link
Collaborator

rwgk commented Aug 27, 2024

Oh! — I only now got to look at the "Files changed" here, and only now realized what this PR is doing. It's orthogonal to the smart_holder change. I think this PR is really useful!

@rwgk
Copy link
Collaborator

rwgk commented Aug 27, 2024

P.S. The title of this PR is unfortunate: what is "custom"? I misunderstood it at first glance.

I recommend working on the PR description, to concisely and carefully state what this PR is about, with a simple example.

Not sure what to write in the title exactly, but I'd definitely try to avoid a vague term like "custom".

Usually I write the PR description first, then extract a title from that.

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