From 05bd93543ba1a75411190e768cb0e6e8880c7acb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 3 Jun 2021 17:09:30 -0700 Subject: [PATCH] Fully emulating type_caster_base-related behavior: trying shared_from_this also for unowned pointees. --- include/pybind11/detail/smart_holder_poc.h | 11 ---- .../detail/smart_holder_type_casters.h | 51 ++++++++++--------- tests/test_class_sh_shared_from_this.py | 7 ++- 3 files changed, 31 insertions(+), 38 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 0448c1d14..36c873dce 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -101,16 +101,6 @@ inline bool is_std_default_delete(const std::type_info &rtti_deleter) { || rtti_deleter == typeid(std::default_delete); } -inline void enable_shared_from_this_from_raw_ptr_take_ownership_guard(...) {} - -template -inline void enable_shared_from_this_from_raw_ptr_take_ownership_guard( - const std::enable_shared_from_this *) { - // This static_assert will always trigger if this template function is instantiated. - static_assert(!std::is_base_of, AnyBaseOfT>::value, - "Ownership must not be transferred via a raw pointer."); -} - struct smart_holder { const std::type_info *rtti_uqp_del = nullptr; std::shared_ptr vptr; @@ -245,7 +235,6 @@ struct smart_holder { template static smart_holder from_raw_ptr_take_ownership(T *raw_ptr) { - enable_shared_from_this_from_raw_ptr_take_ownership_guard(raw_ptr); ensure_pointee_is_destructible("from_raw_ptr_take_ownership"); smart_holder hld; hld.vptr.reset(raw_ptr, make_guarded_builtin_delete(true)); diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index d59e679c6..366474d2e 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -273,25 +273,22 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag using holder_type = pybindit::memory::smart_holder; template - static void from_raw_pointer_take_ownership_or_shared_from_this( - holder_type *uninitialized_location, WrappedType *value_ptr, ...) { - new (uninitialized_location) - holder_type(holder_type::from_raw_ptr_take_ownership(value_ptr)); + static bool try_initialization_using_shared_from_this(holder_type *, WrappedType *, ...) { + return false; } template - static void from_raw_pointer_take_ownership_or_shared_from_this( + static bool try_initialization_using_shared_from_this( holder_type *uninitialized_location, - WrappedType *value_ptr, + WrappedType *value_ptr_w_t, const std::enable_shared_from_this *) { - auto shd_ptr - = std::dynamic_pointer_cast(detail::try_get_shared_from_this(value_ptr)); - if (shd_ptr) { - new (uninitialized_location) holder_type(holder_type::from_shared_ptr(shd_ptr)); - } else { - new (uninitialized_location) - holder_type(holder_type::from_shared_ptr(std::shared_ptr(value_ptr))); - } + auto shd_ptr = std::dynamic_pointer_cast( + detail::try_get_shared_from_this(value_ptr_w_t)); + if (!shd_ptr) + return false; + // Note: inst->owned ignored. + new (uninitialized_location) holder_type(holder_type::from_shared_ptr(shd_ptr)); + return true; } template @@ -305,21 +302,26 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag register_instance(inst, v_h.value_ptr(), v_h.type); v_h.set_instance_registered(); } + auto uninitialized_location = std::addressof(v_h.holder()); + auto value_ptr_w_t = v_h.value_ptr(); if (holder_void_ptr) { // Note: inst->owned ignored. auto holder_ptr = static_cast(holder_void_ptr); - new (std::addressof(v_h.holder())) holder_type(std::move(*holder_ptr)); - } else if (inst->owned) { - from_raw_pointer_take_ownership_or_shared_from_this( - std::addressof(v_h.holder()), - v_h.value_ptr(), - v_h.value_ptr()); + new (uninitialized_location) holder_type(std::move(*holder_ptr)); } else { - new (std::addressof(v_h.holder())) - holder_type(holder_type::from_raw_ptr_unowned(v_h.value_ptr())); + if (!try_initialization_using_shared_from_this( + uninitialized_location, value_ptr_w_t, value_ptr_w_t)) { + if (inst->owned) { + new (uninitialized_location) + holder_type(holder_type::from_raw_ptr_take_ownership(value_ptr_w_t)); + } else { + new (uninitialized_location) + holder_type(holder_type::from_raw_ptr_unowned(value_ptr_w_t)); + } + } } v_h.holder().pointee_depends_on_holder_owner - = dynamic_raw_ptr_cast_if_possible(v_h.value_ptr()) != nullptr; + = dynamic_raw_ptr_cast_if_possible(value_ptr_w_t) != nullptr; v_h.set_holder_constructed(); } @@ -394,6 +396,9 @@ struct smart_holder_type_caster_load { shared_ptr_dec_ref_deleter{ handle((PyObject *) load_impl.loaded_v_h.inst).inc_ref()}); } + if (holder().vptr_is_using_noop_deleter) { + throw std::runtime_error("Non-owning holder (loaded_as_shared_ptr)."); + } std::shared_ptr void_shd_ptr = holder().template as_shared_ptr(); return std::shared_ptr(void_shd_ptr, type_raw_ptr); } diff --git a/tests/test_class_sh_shared_from_this.py b/tests/test_class_sh_shared_from_this.py index 28badf750..5dd0234d4 100644 --- a/tests/test_class_sh_shared_from_this.py +++ b/tests/test_class_sh_shared_from_this.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -# import pytest +import pytest from pybind11_tests import class_sh_shared_from_this as m from pybind11_tests import ConstructorStats @@ -53,10 +53,9 @@ def test_shared_from_this_bad_wp(): bad_wp = s.bad_wp # init_holder_helper(holder_ptr=false, owned=false, bad_wp=true) assert stats.alive() == 2 assert s.set_ref(bad_wp) - # with pytest.raises(RuntimeError) as excinfo: - if 1: # XXX XXX XXX + with pytest.raises(RuntimeError) as excinfo: assert s.set_holder(bad_wp) - # assert "Unable to cast from non-held to held instance" in str(excinfo.value) + assert str(excinfo.value) == "Non-owning holder (loaded_as_shared_ptr)." del bad_wp, s assert stats.alive() == 0