Allow nonconstructible holders#2067
Open
pd-robgee wants to merge 6 commits into
Open
Conversation
Fixes pybind#1178 It's possible for a non-destructible base class to be declared; such a holder *can* be obtained via `std::enable_shared_from_this`, but a shared_ptr cannot be constructed from a returned pointer. This commit puts the holder constructor behind a `std::is_destructible` check, giving a runtime failure if we are ever given such a pointer without a valid `shared_from_this()` shared pointer.
…struct from existing holder object, if possible'
… with another holder type should still be a compile error. Manually caught and converted exception to Python exception, otherwise it escapes and std::terminate() is called.
Known issue: MSVC incorrectly rejects "enable_if_t = 0" SFINAE https://developercommunity.visualstudio.com/content/problem/848104/msvc-incorrectly-rejects-enable-if-t-0-sfinae.html
|
Sorry to interrupt, but is there any progress on this issue? I feel like I'm facing the problem which could be easily fixed as soom as this PR will be merged. Best regards. |
Author
|
Unfortunately the project maintainers didn't/don't seem very interested in incorporating this suggestion into their code base. As can be seen from this PRs conversation page I requested a review from three suggested reviewers, but to this day none of them have even commented anything at all. I have accepted this fate and worked around it by incorporating this patch via the build system in the project I used pybind11 in. I have since moved on to other projects and I'm no longer involved in that project. Therefore, attempting to get this PR merged is no longer a high priority for me. I'm sorry @wanfranck, but thank you for your interest in this patch anyway! |
Collaborator
tomba
added a commit
to tomba/libcamera
that referenced
this pull request
Jan 24, 2022
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Looks like there's a suggested fix, which has not been merged: pybind/pybind11#2067 Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
davidplowman
pushed a commit
to davidplowman/libcamera
that referenced
this pull request
Jan 27, 2022
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Looks like there's a suggested fix, which has not been merged: pybind/pybind11#2067 Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
naushir
pushed a commit
to naushir/libcamera
that referenced
this pull request
Mar 3, 2022
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Looks like there's a suggested fix, which has not been merged: pybind/pybind11#2067 Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
naushir
pushed a commit
to naushir/libcamera
that referenced
this pull request
Mar 4, 2022
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Looks like there's a suggested fix, which has not been merged: pybind/pybind11#2067 Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Collaborator
Have you tried the smart_holder branch? It's basically drop-in, use |
davidplowman
pushed a commit
to raspberrypi/libcamera
that referenced
this pull request
Mar 7, 2022
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Looks like there's a suggested fix, which has not been merged: pybind/pybind11#2067 Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
davidplowman
pushed a commit
to raspberrypi/libcamera
that referenced
this pull request
Mar 31, 2022
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Looks like there's a suggested fix, which has not been merged: pybind/pybind11#2067 Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
davidplowman
pushed a commit
to raspberrypi/libcamera
that referenced
this pull request
Apr 26, 2022
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Looks like there's a suggested fix, which has not been merged: pybind/pybind11#2067 Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
jhautbois
pushed a commit
to jhautbois/libcamera
that referenced
this pull request
May 2, 2022
pybind11 needs a public destructor for Camera to be able to manage the shared_ptr. It's not clear why this is needed, and can it be fixed in pybind11. Looks like there's a suggested fix, which has not been merged: pybind/pybind11#2067 Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
kbingham
pushed a commit
to kbingham/libcamera
that referenced
this pull request
May 10, 2022
Add libcamera Python bindings. pybind11 is used to generate the C++ <-> Python layer. We use pybind11 'smart_holder' version to avoid issues with private destructors and shared_ptr. There is also an alternative solution here: pybind/pybind11#2067 Only a subset of libcamera classes are exposed. Implementing and testing the wrapper classes is challenging, and as such only classes that I have needed have been added so far. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1178
This PR implements the fix from 53be819.
That fix by itself leads to an uncaught exception instead of a Python RuntimeError, this is fixed in this PR.
Furthermore test cases have been added.