From 64716cc14ce4e8a83ee1326995edfae8164f4f55 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 24 Jun 2021 15:42:56 -0700 Subject: [PATCH] Adding a few comments after review by @laramiel. --- include/pybind11/detail/init.h | 9 +++++++-- tests/test_class_sh_trampoline_shared_from_this.py | 7 +++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index 9f086d1b4..faf6e14a5 100644 --- a/include/pybind11/detail/init.h +++ b/include/pybind11/detail/init.h @@ -182,8 +182,13 @@ 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"); + // Here and below: if the new object is a trampoline, the shared_from_this mechanism needs + // to be prevented from accessing the smart_holder vptr, because it does not keep the + // trampoline Python object alive. For types that don't inherit from enable_shared_from_this + // it does not matter if void_cast_raw_ptr is true or false, therefore it's not necessary + // to also inspect the type. auto smhldr = type_caster>::template smart_holder_from_unique_ptr( - std::move(unq_ptr), Class::has_alias && is_alias(ptr)); + std::move(unq_ptr), /*void_cast_raw_ptr*/ Class::has_alias && is_alias(ptr)); v_h.value_ptr() = ptr; v_h.type->init_instance(v_h.inst, &smhldr); } @@ -198,7 +203,7 @@ void construct(value_and_holder &v_h, auto *ptr = unq_ptr.get(); no_nullptr(ptr); auto smhldr = type_caster>::template smart_holder_from_unique_ptr( - std::move(unq_ptr), true); + std::move(unq_ptr), /*void_cast_raw_ptr*/ true); v_h.value_ptr() = ptr; v_h.type->init_instance(v_h.inst, &smhldr); } diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py index 6152d0542..12ae5c76d 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.py +++ b/tests/test_class_sh_trampoline_shared_from_this.py @@ -9,6 +9,8 @@ class PySft(m.Sft): def test_release_and_shared_from_this(): + # Exercises the most direct path from building a shared_from_this-visible + # shared_ptr to calling shared_from_this. obj = PySft("PySft") assert obj.history == "PySft" assert m.use_count(obj) == 1 @@ -30,6 +32,7 @@ def test_release_and_shared_from_this_leak(): def test_release_and_stash(): + # Exercises correct functioning of guarded_delete weak_ptr. obj = PySft("PySft") stash1 = m.SftSharedPtrStash(1) stash1.Add(obj) @@ -88,6 +91,7 @@ def test_release_and_stash_leak(): def test_release_and_stash_via_shared_from_this(): + # Exercises that the smart_holder vptr is invisible to the shared_from_this mechnism. obj = PySft("PySft") stash1 = m.SftSharedPtrStash(1) with pytest.raises(RuntimeError) as exc_info: @@ -120,6 +124,8 @@ def test_release_and_stash_via_shared_from_this_leak(): def test_pass_released_shared_ptr_as_unique_ptr(): + # Exercises that returning a unique_ptr fails while a shared_from_this + # visible shared_ptr exists. obj = PySft("PySft") stash1 = m.SftSharedPtrStash(1) stash1.Add(obj) # Releases shared_ptr to C++. @@ -139,6 +145,7 @@ def test_pass_released_shared_ptr_as_unique_ptr(): ], ) def test_pure_cpp_sft_raw_ptr(make_f): + # Exercises void_cast_raw_ptr logic for different situations. obj = make_f("PureCppSft") assert m.pass_shared_ptr(obj) == 3 assert obj.history == "PureCppSft_PassSharedPtr"