fix: add support for shared_ptr<const T> in py::init() with smart_holder
#5731
Conversation
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
….def(py::init(&rtrn_uqcp))` and `.def(py::init(&rtrn_udcp))`, which happen to work already (not sure what change in the past made those work).
There was a problem hiding this comment.
Pull Request Overview
This PR enables std::shared_ptr<const T> support in py::init() when using py::smart_holder, complementing existing std::unique_ptr<const T> support by:
- Introducing a
construct_from_shared_ptrhelper and overloads forshared_ptr<const Cpp<Class>>ininit.h. - Uncommenting and activating related Python tests in
test_class_sh_factory_constructors.py. - Enabling corresponding C++ factory constructor bindings in
test_class_sh_factory_constructors.cpp.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| include/pybind11/detail/init.h | Added construct_from_shared_ptr and overload for shared_ptr<const> |
| tests/test_class_sh_factory_constructors.py | Uncommented assertions for shared_ptr<const> and unique_ptr<const> |
| tests/test_class_sh_factory_constructors.cpp | Removed commented-out .def(py::init(...)) lines and enabled constructors |
Comments suppressed due to low confidence (3)
tests/test_class_sh_factory_constructors.cpp:110
- [nitpick] Remove this stale commented-out block to keep the test concise and focused on the enabled constructor.
py::classh<atyp_shcp>(m, "atyp_shcp")
tests/test_class_sh_factory_constructors.cpp:118
- [nitpick] This comment is no longer needed once the constructor is enabled; consider deleting it to reduce noise.
py::classh<atyp_uqcp>(m, "atyp_uqcp")
tests/test_class_sh_factory_constructors.cpp:126
- [nitpick] Stale commented code can be removed now that the smart_holder-based init is active for this type.
py::classh<atyp_udcp>(m, "atyp_udcp")
| @@ -13,11 +13,11 @@ def test_atyp_factories(): | |||
| # sert m.atyp_cptr().get_mtxt() == "Cptr" | |||
There was a problem hiding this comment.
There's a typo in the comment: change "sert" to "assert" to match the style of the other lines.
Copilot uses AI. Check for mistakes.
henryiii
approved these changes
Jun 17, 2025
henryiii
changed the title
shared_ptr<const T> in py::init() with smart_holdershared_ptr<const T> in py::init() with smart_holder
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Experiments related to PR #5718 triggered a review of factory constructor support in combination with
py::smart_holder.This PR adds support for
std::shared_ptr<const T>inpy::init()by introducing a minimalconstruct()overload withconst_pointer_cast.No change is needed for
std::unique_ptr<const T>, which is already supported by existing overloads via template deduction (this just went unnoticed before).Uncommenting related test code in in
test_class_sh_factory_constructors.pyis all that's needed to activate corresponding runtime coverage.Suggested changelog entry:
std::shared_ptr<const T>inpy::init()when usingpy::smart_holder, complementing existing support forstd::unique_ptr<const T>.