Adding reclaim_disowned logic & miscellaneous naming and documentation improvements. (#2943)

* Using new smart_holder::reclaim_disowned in smart_holder_type_caster for unique_ptr.

* Systematically renaming was_disowned to is_disowned (because disowning is now reversible: reclaim_disowned).

* Systematically renaming virtual_overrider_self_life_support to trampoline_self_life_support (to reuse existing terminology instead of introducing new one).

* Systematically renaming test_class_sh_with_alias to test_class_sh_trampoline_basic.

* Adding a Trampolines and std::unique_ptr section to README_smart_holder.rst.

* MSVC compatibility.

tests/CMakeLists.txt

@@ -106,11 +106,11 @@ set(PYBIND11_TEST_FILES
     test_class_sh_disowning_mi.cpp
     test_class_sh_factory_constructors.cpp
     test_class_sh_inheritance.cpp
+    test_class_sh_trampoline_basic.cpp
     test_class_sh_trampoline_shared_ptr_cpp_arg.cpp
     test_class_sh_trampoline_unique_ptr.cpp
     test_class_sh_unique_ptr_member.cpp
     test_class_sh_virtual_py_cpp_mix.cpp
-    test_class_sh_with_alias.cpp
     test_constants_and_functions.cpp
     test_copy_move.cpp
     test_custom_type_casters.cpp

tests/extra_python_package/test_files.py

@@ -35,7 +35,7 @@ main_headers = {
     "include/pybind11/smart_holder.h",
     "include/pybind11/stl.h",
     "include/pybind11/stl_bind.h",
-    "include/pybind11/virtual_overrider_self_life_support.h",
+    "include/pybind11/trampoline_self_life_support.h",
 }
 
 detail_headers = {

tests/pure_cpp/smart_holder_poc_test.cpp

@@ -129,6 +129,20 @@ TEST_CASE("from_raw_ptr_take_ownership+as_shared_ptr", "[S]") {
     REQUIRE(*new_owner == 19);
 }
 
+TEST_CASE("from_raw_ptr_take_ownership+disown+reclaim_disowned", "[S]") {
+    auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
+    std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
+    hld.disown();
+    REQUIRE(hld.as_lvalue_ref<int>() == 19);
+    REQUIRE(*new_owner == 19);
+    hld.reclaim_disowned(); // Manually veriified: without this, clang++ -fsanitize=address reports
+                            // "detected memory leaks".
+    new_owner.release();    // Manually verified: without this, clang++ -fsanitize=address reports
+                            // "attempting double-free".
+    REQUIRE(hld.as_lvalue_ref<int>() == 19);
+    REQUIRE(new_owner.get() == nullptr);
+}
+
 TEST_CASE("from_raw_ptr_take_ownership+disown+release_disowned", "[S]") {
     auto hld = smart_holder::from_raw_ptr_take_ownership(new int(19));
     std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
@@ -139,13 +153,13 @@ TEST_CASE("from_raw_ptr_take_ownership+disown+release_disowned", "[S]") {
     REQUIRE(!hld.has_pointee());
 }
 
-TEST_CASE("from_raw_ptr_take_ownership+disown+ensure_was_not_disowned", "[E]") {
+TEST_CASE("from_raw_ptr_take_ownership+disown+ensure_is_not_disowned", "[E]") {
     const char *context = "test_case";
     auto hld            = smart_holder::from_raw_ptr_take_ownership(new int(19));
-    hld.ensure_was_not_disowned(context); // Does not throw.
+    hld.ensure_is_not_disowned(context); // Does not throw.
     std::unique_ptr<int> new_owner(hld.as_raw_ptr_unowned<int>());
     hld.disown();
-    REQUIRE_THROWS_WITH(hld.ensure_was_not_disowned(context),
+    REQUIRE_THROWS_WITH(hld.ensure_is_not_disowned(context),
                         "Holder was disowned already (test_case).");
 }
 

tests/test_class_sh_disowning.py

@@ -41,7 +41,7 @@ def test_mixed():
             # the already disowned obj1a fails as expected.
             m.mixed(obj1a, obj2b)
 
-        def was_disowned(obj):
+        def is_disowned(obj):
             try:
                 obj.get()
             except ValueError:
@@ -50,13 +50,13 @@ def test_mixed():
 
         # Either obj1b or obj2b was disowned in the expected failed m.mixed() calls above, but not
         # both.
-        was_disowned_results = (was_disowned(obj1b), was_disowned(obj2b))
-        assert was_disowned_results.count(True) == 1
+        is_disowned_results = (is_disowned(obj1b), is_disowned(obj2b))
+        assert is_disowned_results.count(True) == 1
         if first_pass:
             first_pass = False
             print(
                 "\nC++ function argument %d is evaluated first."
-                % (was_disowned_results.index(True) + 1)
+                % (is_disowned_results.index(True) + 1)
             )
 
         return  # Comment out for manual leak checking (use `top` command).

tests/test_class_sh_disowning_mi.py

@@ -18,7 +18,7 @@ def test_diamond_inheritance():
     assert d is d.c0().c1().b().c0().b()
 
 
-def was_disowned(callable_method):
+def is_disowned(callable_method):
     try:
         callable_method()
     except ValueError as e:
@@ -34,7 +34,7 @@ def test_disown_b():
     b = m.B()
     assert b.get() == 10
     m.disown_b(b)
-    assert was_disowned(b.get)
+    assert is_disowned(b.get)
 
 
 @pytest.mark.parametrize("var_to_disown", ["c0", "b"])
@@ -43,8 +43,8 @@ def test_disown_c0(var_to_disown):
     assert c0.get() == 1020
     b = c0.b()
     m.disown_b(locals()[var_to_disown])
-    assert was_disowned(c0.get)
-    assert was_disowned(b.get)
+    assert is_disowned(c0.get)
+    assert is_disowned(b.get)
 
 
 @pytest.mark.parametrize("var_to_disown", ["c1", "b"])
@@ -53,8 +53,8 @@ def test_disown_c1(var_to_disown):
     assert c1.get() == 1021
     b = c1.b()
     m.disown_b(locals()[var_to_disown])
-    assert was_disowned(c1.get)
-    assert was_disowned(b.get)
+    assert is_disowned(c1.get)
+    assert is_disowned(b.get)
 
 
 @pytest.mark.parametrize("var_to_disown", ["d", "c1", "c0", "b"])
@@ -65,10 +65,10 @@ def test_disown_d(var_to_disown):
     c0 = d.c0()
     c1 = d.c1()
     m.disown_b(locals()[var_to_disown])
-    assert was_disowned(d.get)
-    assert was_disowned(c1.get)
-    assert was_disowned(c0.get)
-    assert was_disowned(b.get)
+    assert is_disowned(d.get)
+    assert is_disowned(c1.get)
+    assert is_disowned(c0.get)
+    assert is_disowned(b.get)
 
 
 # Based on test_multiple_inheritance.py:test_multiple_inheritance_python.
@@ -211,10 +211,10 @@ def test_disown_base1_first(cls, i, j, v):
     obj = cls(i, j)
     assert obj.foo() == i
     assert m.disown_base1(obj) == 2000 * i + 1
-    assert was_disowned(obj.foo)
+    assert is_disowned(obj.foo)
     assert obj.bar() == j
     assert m.disown_base2(obj) == 2000 * j + 2
-    assert was_disowned(obj.bar)
+    assert is_disowned(obj.bar)
     if v is not None:
         assert obj.v() == v
 
@@ -226,10 +226,10 @@ def test_disown_base2_first(cls, i, j, v):
     obj = cls(i, j)
     assert obj.bar() == j
     assert m.disown_base2(obj) == 2000 * j + 2
-    assert was_disowned(obj.bar)
+    assert is_disowned(obj.bar)
     assert obj.foo() == i
     assert m.disown_base1(obj) == 2000 * i + 1
-    assert was_disowned(obj.foo)
+    assert is_disowned(obj.foo)
     if v is not None:
         assert obj.v() == v
 
@@ -249,5 +249,5 @@ def test_disown_base2(cls, j, v):
     obj = cls(j)
     assert obj.bar() == j
     assert m.disown_base2(obj) == 2000 * j + 2
-    assert was_disowned(obj.bar)
+    assert is_disowned(obj.bar)
     assert obj.v() == v

tests/test_class_sh_trampoline_basic.cpp

@@ -5,7 +5,7 @@
 #include <memory>
 
 namespace pybind11_tests {
-namespace class_sh_with_alias {
+namespace class_sh_trampoline_basic {
 
 template <int SerNo> // Using int as a trick to easily generate a series of types.
 struct Abase {
@@ -35,7 +35,7 @@ struct AbaseAlias : Abase<SerNo> {
 };
 
 template <>
-struct AbaseAlias<1> : Abase<1>, py::virtual_overrider_self_life_support {
+struct AbaseAlias<1> : Abase<1>, py::trampoline_self_life_support {
     using Abase<1>::Abase;
 
     int Add(int other_val) const override {
@@ -73,14 +73,14 @@ void wrap(py::module_ m, const char *py_class_name) {
     m.def("AddInCppUniquePtr", AddInCppUniquePtr<SerNo>, py::arg("obj"), py::arg("other_val"));
 }
 
-} // namespace class_sh_with_alias
+} // namespace class_sh_trampoline_basic
 } // namespace pybind11_tests
 
-PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_with_alias::Abase<0>)
-PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_with_alias::Abase<1>)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_trampoline_basic::Abase<0>)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_trampoline_basic::Abase<1>)
 
-TEST_SUBMODULE(class_sh_with_alias, m) {
-    using namespace pybind11_tests::class_sh_with_alias;
+TEST_SUBMODULE(class_sh_trampoline_basic, m) {
+    using namespace pybind11_tests::class_sh_trampoline_basic;
     wrap<0>(m, "Abase0");
     wrap<1>(m, "Abase1");
 }

tests/test_class_sh_trampoline_basic.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 import pytest
 
-from pybind11_tests import class_sh_with_alias as m
+from pybind11_tests import class_sh_trampoline_basic as m
 
 
 class PyDrvd0(m.Abase0):
@@ -45,7 +45,7 @@ def test_drvd0_add_in_cpp_unique_ptr():
         assert (
             str(exc_info.value)
             == "Alias class (also known as trampoline) does not inherit from"
-            " py::virtual_overrider_self_life_support, therefore the ownership of this"
+            " py::trampoline_self_life_support, therefore the ownership of this"
             " instance cannot safely be transferred to C++."
         )
         return  # Comment out for manual leak checking (use `top` command).

tests/test_class_sh_trampoline_unique_ptr.cpp

@@ -3,14 +3,20 @@
 // BSD-style license that can be found in the LICENSE file.
 
 #include "pybind11/smart_holder.h"
-#include "pybind11/virtual_overrider_self_life_support.h"
+#include "pybind11/trampoline_self_life_support.h"
 #include "pybind11_tests.h"
 
+#include <cstdint>
+
 namespace {
 
 class Class {
 public:
-    virtual ~Class()                             = default;
+    virtual ~Class() = default;
+
+    void setVal(std::uint64_t val) { val_ = val; }
+    std::uint64_t getVal() const { return val_; }
+
     virtual std::unique_ptr<Class> clone() const = 0;
     virtual int foo() const                      = 0;
 
@@ -19,6 +25,9 @@ protected:
 
     // Some compilers complain about implicitly defined versions of some of the following:
     Class(const Class &) = default;
+
+private:
+    std::uint64_t val_ = 0;
 };
 
 } // namespace
@@ -27,7 +36,7 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(Class)
 
 namespace {
 
-class PyClass : public Class, public py::virtual_overrider_self_life_support {
+class PyClass : public Class, public py::trampoline_self_life_support {
 public:
     std::unique_ptr<Class> clone() const override {
         PYBIND11_OVERRIDE_PURE(std::unique_ptr<Class>, Class, clone);
@@ -41,8 +50,11 @@ public:
 TEST_SUBMODULE(class_sh_trampoline_unique_ptr, m) {
     py::classh<Class, PyClass>(m, "Class")
         .def(py::init<>())
+        .def("set_val", &Class::setVal)
+        .def("get_val", &Class::getVal)
         .def("clone", &Class::clone)
         .def("foo", &Class::foo);
 
+    m.def("clone", [](const Class &obj) { return obj.clone(); });
     m.def("clone_and_foo", [](const Class &obj) { return obj.clone()->foo(); });
 }

tests/test_class_sh_trampoline_unique_ptr.py

@@ -5,13 +5,27 @@ import pybind11_tests.class_sh_trampoline_unique_ptr as m
 
 class MyClass(m.Class):
     def foo(self):
-        return 10
+        return 10 + self.get_val()
 
     def clone(self):
-        return MyClass()
+        cloned = MyClass()
+        cloned.set_val(self.get_val() + 3)
+        return cloned
 
 
-def test_py_clone_and_foo():
+def test_m_clone():
     obj = MyClass()
-    assert obj.foo() == 10
-    assert m.clone_and_foo(obj) == 10
+    while True:
+        obj.set_val(5)
+        obj = m.clone(obj)
+        assert obj.get_val() == 5 + 3
+        assert obj.foo() == 10 + 5 + 3
+        return  # Comment out for manual leak checking (use `top` command).
+
+
+def test_m_clone_and_foo():
+    obj = MyClass()
+    obj.set_val(7)
+    while True:
+        assert m.clone_and_foo(obj) == 10 + 7 + 3
+        return  # Comment out for manual leak checking (use `top` command).

tests/test_class_sh_virtual_py_cpp_mix.cpp

@@ -31,13 +31,13 @@ int get_from_cpp_plainc_ptr(const Base *b) { return b->get() + 4000; }
 
 int get_from_cpp_unique_ptr(std::unique_ptr<Base> b) { return b->get() + 5000; }
 
-struct BaseVirtualOverrider : Base, py::virtual_overrider_self_life_support {
+struct BaseVirtualOverrider : Base, py::trampoline_self_life_support {
     using Base::Base;
 
     int get() const override { PYBIND11_OVERRIDE(int, Base, get); }
 };
 
-struct CppDerivedVirtualOverrider : CppDerived, py::virtual_overrider_self_life_support {
+struct CppDerivedVirtualOverrider : CppDerived, py::trampoline_self_life_support {
     using CppDerived::CppDerived;
 
     int get() const override { PYBIND11_OVERRIDE(int, CppDerived, get); }