-
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
Smart Pointers for non custom types #1985
base: master
Are you sure you want to change the base?
Conversation
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 I did consider making the different versions of 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.
|
I just realized my implementation has some issues with the correct |
I think this fixed the return value policy but I think some review here if the If you have a custom binding for a class |
@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! |
@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. |
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 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 |
@rwgk Do you have plans to merge it in master? |
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! |
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. |
Added a way for non custom types to take advantage of smart pointers which hopefully solves #787.