From 146a925e4ecff04d49790394db4283c8a4a3a44e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 24 Jun 2021 06:04:34 -0700 Subject: [PATCH] Resolving TODOs for `from_unique_ptr` `void_cast_raw_ptr`. Adding test to exercise the path through `cast`. The path through init.h is missing a test that would fail if the flag is incorrect. The plan is to systematically cover all situations (there are many that are not exercised for shared_from_this). --- include/pybind11/detail/init.h | 8 ++++---- include/pybind11/detail/smart_holder_poc.h | 6 +++++- include/pybind11/detail/smart_holder_type_casters.h | 13 +++++++------ tests/test_class_sh_trampoline_shared_from_this.cpp | 5 +++++ tests/test_class_sh_trampoline_shared_from_this.py | 7 ++++++- 5 files changed, 27 insertions(+), 12 deletions(-) diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index 0aea1f52c..9f086d1b4 100644 --- a/include/pybind11/detail/init.h +++ b/include/pybind11/detail/init.h @@ -182,8 +182,8 @@ void construct(value_and_holder &v_h, std::unique_ptr, D> &&unq_ptr, if (Class::has_alias && need_alias && !is_alias(ptr)) throw type_error("pybind11::init(): construction failed: returned std::unique_ptr pointee " "is not an alias instance"); - auto smhldr - = type_caster>::template smart_holder_from_unique_ptr(std::move(unq_ptr)); + auto smhldr = type_caster>::template smart_holder_from_unique_ptr( + std::move(unq_ptr), Class::has_alias && is_alias(ptr)); v_h.value_ptr() = ptr; v_h.type->init_instance(v_h.inst, &smhldr); } @@ -197,8 +197,8 @@ void construct(value_and_holder &v_h, bool /*need_alias*/) { auto *ptr = unq_ptr.get(); no_nullptr(ptr); - auto smhldr - = type_caster>::template smart_holder_from_unique_ptr(std::move(unq_ptr)); + auto smhldr = type_caster>::template smart_holder_from_unique_ptr( + std::move(unq_ptr), true); v_h.value_ptr() = ptr; v_h.type->init_instance(v_h.inst, &smhldr); } diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 48805069f..223e1b1f3 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -40,6 +40,10 @@ Details: * By choice, the smart_holder is movable but not copyable, to keep the design simple, and to guard against accidental copying overhead. + +* The `void_cast_raw_ptr` option is needed to make the `smart_holder` `vptr` + member invisible to the `shared_from_this` mechanism, in case the lifetime + of a `PyObject` is tied to the pointee. */ #pragma once @@ -51,7 +55,7 @@ Details: #include #include -//#include +// #include // inline void to_cout(const std::string &msg) { std::cout << msg << std::endl; } inline void to_cout(const std::string &) {} diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 913951ad7..a906ae642 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -317,7 +317,7 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag if (inst->owned) { new (uninitialized_location) holder_type(holder_type::from_raw_ptr_take_ownership( - value_ptr_w_t, pointee_depends_on_holder_owner)); + value_ptr_w_t, /*void_cast_raw_ptr*/ pointee_depends_on_holder_owner)); } else { new (uninitialized_location) holder_type(holder_type::from_raw_ptr_unowned(value_ptr_w_t)); @@ -330,9 +330,10 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag } template - static smart_holder smart_holder_from_unique_ptr(std::unique_ptr &&unq_ptr) { - return pybindit::memory::smart_holder::from_unique_ptr( - std::move(unq_ptr), /*TODO pointee_depends_on_holder_owner*/ true); + static smart_holder smart_holder_from_unique_ptr(std::unique_ptr &&unq_ptr, + bool void_cast_raw_ptr) { + return pybindit::memory::smart_holder::from_unique_ptr(std::move(unq_ptr), + void_cast_raw_ptr); } template @@ -805,8 +806,8 @@ struct smart_holder_type_caster> : smart_holder_type_caste void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr(); valueptr = src_raw_void_ptr; - auto smhldr = pybindit::memory::smart_holder::from_unique_ptr( - std::move(src), /*TODO pointee_depends_on_holder_owner*/ true); + auto smhldr = pybindit::memory::smart_holder::from_unique_ptr(std::move(src), + /*void_cast_raw_ptr*/ false); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); if (policy == return_value_policy::reference_internal) diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp index 49dfb036c..81024dc6f 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.cpp +++ b/tests/test_class_sh_trampoline_shared_from_this.cpp @@ -89,6 +89,10 @@ void pass_unique_ptr(const std::unique_ptr &) {} Sft *make_pure_cpp_sft_raw_ptr(const std::string &history_seed) { return new Sft{history_seed}; } +std::unique_ptr make_pure_cpp_sft_unq_ptr(const std::string &history_seed) { + return std::unique_ptr(new Sft{history_seed}); +} + std::shared_ptr make_pure_cpp_sft_shd_ptr(const std::string &history_seed) { return std::make_shared(history_seed); } @@ -115,6 +119,7 @@ TEST_SUBMODULE(class_sh_trampoline_shared_from_this, m) { m.def("pass_shared_ptr", pass_shared_ptr); m.def("pass_unique_ptr", pass_unique_ptr); m.def("make_pure_cpp_sft_raw_ptr", make_pure_cpp_sft_raw_ptr); + m.def("make_pure_cpp_sft_unq_ptr", make_pure_cpp_sft_unq_ptr); m.def("make_pure_cpp_sft_shd_ptr", make_pure_cpp_sft_shd_ptr); m.def("to_cout", to_cout); } diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py index 665123d75..6152d0542 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.py +++ b/tests/test_class_sh_trampoline_shared_from_this.py @@ -131,7 +131,12 @@ def test_pass_released_shared_ptr_as_unique_ptr(): @pytest.mark.parametrize( - "make_f", [m.make_pure_cpp_sft_raw_ptr, m.make_pure_cpp_sft_shd_ptr] + "make_f", + [ + m.make_pure_cpp_sft_raw_ptr, + m.make_pure_cpp_sft_unq_ptr, + m.make_pure_cpp_sft_shd_ptr, + ], ) def test_pure_cpp_sft_raw_ptr(make_f): obj = make_f("PureCppSft")