From 716a2952d264c648da0815ef16740ad8e9bd4836 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 2 Dec 2022 08:52:59 -0800 Subject: [PATCH 01/15] Content of PR #4374 applied on top of smart_holder branch. --- tests/CMakeLists.txt | 1 + tests/test_mi_debug.cpp | 53 +++++++++++++++++++++++++++++++++++++++++ tests/test_mi_debug.py | 8 +++++++ 3 files changed, 62 insertions(+) create mode 100644 tests/test_mi_debug.cpp create mode 100644 tests/test_mi_debug.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d958a8c5ca..fa185a2f8e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -158,6 +158,7 @@ set(PYBIND11_TEST_FILES test_kwargs_and_defaults test_local_bindings test_methods_and_attributes + test_mi_debug test_modules test_multiple_inheritance test_numpy_array diff --git a/tests/test_mi_debug.cpp b/tests/test_mi_debug.cpp new file mode 100644 index 0000000000..c475314ca3 --- /dev/null +++ b/tests/test_mi_debug.cpp @@ -0,0 +1,53 @@ +#include + +#include "pybind11_tests.h" + +#include +#include +#include + +// The first base class. +struct Base0 { + virtual ~Base0() = default; +}; + +using Base0Ptr = std::shared_ptr; + +// The second base class. +struct Base1 { + virtual ~Base1() = default; + std::vector vec = {1, 2, 3, 4, 5}; +}; + +using Base1Ptr = std::shared_ptr; + +// The derived class. +struct Derived : Base1, Base0 { + ~Derived() override = default; +}; + +using DerivedPtr = std::shared_ptr; + +TEST_SUBMODULE(mi_debug, m) { + // Expose the bases. + pybind11::class_ bs0(m, "Base0"); + pybind11::class_ bs1(m, "Base1"); + // Expose the derived class. + pybind11::class_(m, "Derived").def(pybind11::init<>()); + + // A helper that returns a pointer to base. + m.def("make_object", []() -> Base0Ptr { + auto ret_der = std::make_shared(); + std::cout << "ret der ptr: " << ret_der.get() << std::endl; + auto ret = Base0Ptr(ret_der); + std::cout << "ret base ptr: " << ret.get() << std::endl; + return ret; + }); + + // A helper that accepts in input a pointer to derived. + m.def("get_object_vec_size", [](const DerivedPtr &object) { + std::cout << "der ptr: " << object.get() << std::endl; + std::cout << object->vec.size() << std::endl; + return object->vec.size(); + }); +} diff --git a/tests/test_mi_debug.py b/tests/test_mi_debug.py new file mode 100644 index 0000000000..d4acc87653 --- /dev/null +++ b/tests/test_mi_debug.py @@ -0,0 +1,8 @@ +import pytest + +m = pytest.importorskip("pybind11_tests.mi_debug") + + +def test_vec(): + o = m.make_object() + assert 5 == m.get_object_vec_size(o) From a2d5b9e44e346fce632893e951ce9f5cb442d139 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 2 Dec 2022 11:16:12 -0800 Subject: [PATCH 02/15] More tests, with USE_SH switch. [ci skip] --- tests/test_mi_debug.cpp | 72 +++++++++++++++++++++++++++-------------- tests/test_mi_debug.py | 18 ++++++++--- 2 files changed, 60 insertions(+), 30 deletions(-) diff --git a/tests/test_mi_debug.cpp b/tests/test_mi_debug.cpp index c475314ca3..7079efb9c8 100644 --- a/tests/test_mi_debug.cpp +++ b/tests/test_mi_debug.cpp @@ -1,53 +1,75 @@ #include +#define USE_SH + +#if defined(USE_SH) +# include +#endif + #include "pybind11_tests.h" -#include +#include #include #include -// The first base class. +namespace test_mi_debug { + struct Base0 { virtual ~Base0() = default; }; -using Base0Ptr = std::shared_ptr; - -// The second base class. struct Base1 { virtual ~Base1() = default; std::vector vec = {1, 2, 3, 4, 5}; }; -using Base1Ptr = std::shared_ptr; - -// The derived class. struct Derived : Base1, Base0 { ~Derived() override = default; }; -using DerivedPtr = std::shared_ptr; +} // namespace test_mi_debug + +#if defined(USE_SH) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Base0) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Base1) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Derived) +#endif TEST_SUBMODULE(mi_debug, m) { - // Expose the bases. - pybind11::class_ bs0(m, "Base0"); - pybind11::class_ bs1(m, "Base1"); - // Expose the derived class. - pybind11::class_(m, "Derived").def(pybind11::init<>()); - - // A helper that returns a pointer to base. - m.def("make_object", []() -> Base0Ptr { + using namespace test_mi_debug; + +#if defined(USE_SH) + py::classh bs0(m, "Base0"); + py::classh bs1(m, "Base1"); + py::classh(m, "Derived").def(py::init<>()); +#else + py::class_> bs0(m, "Base0"); + py::class_> bs1(m, "Base1"); + py::class_, Base1, Base0>(m, "Derived").def(py::init<>()); +#endif + + m.def("make_derived_as_base0", []() -> std::shared_ptr { auto ret_der = std::make_shared(); - std::cout << "ret der ptr: " << ret_der.get() << std::endl; - auto ret = Base0Ptr(ret_der); - std::cout << "ret base ptr: " << ret.get() << std::endl; + auto ret = std::shared_ptr(ret_der); return ret; }); - // A helper that accepts in input a pointer to derived. - m.def("get_object_vec_size", [](const DerivedPtr &object) { - std::cout << "der ptr: " << object.get() << std::endl; - std::cout << object->vec.size() << std::endl; - return object->vec.size(); + // class_ OK + // classh FAIL + m.def("get_vec_size_raw_ptr_base0", [](const Base0 *obj) -> std::size_t { + auto obj_der = dynamic_cast(obj); + if (obj_der == nullptr) { + return 0; + } + return obj_der->vec.size(); }); + + // class_ OK + // classh FAIL + m.def("get_vec_size_raw_ptr_derived", [](const Derived *obj) { return obj->vec.size(); }); + + // class_ OK + // classh FAIL + m.def("get_vec_size_shared_ptr_derived", + [](const std::shared_ptr &obj) { return obj->vec.size(); }); } diff --git a/tests/test_mi_debug.py b/tests/test_mi_debug.py index d4acc87653..d2bbe26f2d 100644 --- a/tests/test_mi_debug.py +++ b/tests/test_mi_debug.py @@ -1,8 +1,16 @@ -import pytest +from pybind11_tests import mi_debug as m -m = pytest.importorskip("pybind11_tests.mi_debug") +def test_get_vec_size_raw_ptr_base0(): + obj = m.make_derived_as_base0() + assert m.get_vec_size_raw_ptr_base0(obj) == 5 -def test_vec(): - o = m.make_object() - assert 5 == m.get_object_vec_size(o) + +def test_get_vec_size_raw_ptr_derived(): + obj = m.make_derived_as_base0() + assert m.get_vec_size_raw_ptr_derived(obj) == 5 + + +def test_get_vec_size_shared_ptr_derived(): + obj = m.make_derived_as_base0() + assert m.get_vec_size_shared_ptr_derived(obj) == 5 From 551eabe8e71c8ea704e938988a2c0e17d6f76a57 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 2 Dec 2022 12:05:38 -0800 Subject: [PATCH 03/15] Use `std::dynamic_pointer_cast` [ci skip] --- tests/test_mi_debug.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_mi_debug.cpp b/tests/test_mi_debug.cpp index 7079efb9c8..0285b6440b 100644 --- a/tests/test_mi_debug.cpp +++ b/tests/test_mi_debug.cpp @@ -50,7 +50,7 @@ TEST_SUBMODULE(mi_debug, m) { m.def("make_derived_as_base0", []() -> std::shared_ptr { auto ret_der = std::make_shared(); - auto ret = std::shared_ptr(ret_der); + auto ret = std::dynamic_pointer_cast(ret_der); return ret; }); From af08ec6c66911694b9c482c4cd94949e76ff704d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 2 Dec 2022 14:00:31 -0800 Subject: [PATCH 04/15] All tests pass when using `m.make_derived_as_base0_raw_ptr()`, with `USE_SH` defined or not defined. [ci skip] --- tests/test_mi_debug.cpp | 29 ++++++++++++++++++++++++++++- tests/test_mi_debug.py | 6 +++--- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/tests/test_mi_debug.cpp b/tests/test_mi_debug.cpp index 0285b6440b..7b57c9d7c0 100644 --- a/tests/test_mi_debug.cpp +++ b/tests/test_mi_debug.cpp @@ -16,15 +16,21 @@ namespace test_mi_debug { struct Base0 { virtual ~Base0() = default; + Base0() = default; + Base0(const Base0 &) = delete; }; struct Base1 { virtual ~Base1() = default; std::vector vec = {1, 2, 3, 4, 5}; + Base1() = default; + Base1(const Base1 &) = delete; }; struct Derived : Base1, Base0 { ~Derived() override = default; + Derived() = default; + Derived(const Derived &) = delete; }; } // namespace test_mi_debug @@ -50,10 +56,27 @@ TEST_SUBMODULE(mi_debug, m) { m.def("make_derived_as_base0", []() -> std::shared_ptr { auto ret_der = std::make_shared(); + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) ret_der.get(), __FILE__, __LINE__); + fflush(stdout); auto ret = std::dynamic_pointer_cast(ret_der); + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) ret.get(), __FILE__, __LINE__); + fflush(stdout); return ret; }); + m.def( + "make_derived_as_base0_raw_ptr", + []() { + auto ret_der = new Derived{}; + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) ret_der, __FILE__, __LINE__); + fflush(stdout); + auto ret = dynamic_cast(ret_der); + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) ret, __FILE__, __LINE__); + fflush(stdout); + return ret; + }, + py::return_value_policy::take_ownership); + // class_ OK // classh FAIL m.def("get_vec_size_raw_ptr_base0", [](const Base0 *obj) -> std::size_t { @@ -66,7 +89,11 @@ TEST_SUBMODULE(mi_debug, m) { // class_ OK // classh FAIL - m.def("get_vec_size_raw_ptr_derived", [](const Derived *obj) { return obj->vec.size(); }); + m.def("get_vec_size_raw_ptr_derived", [](const Derived *obj) { + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) obj, __FILE__, __LINE__); + fflush(stdout); + return obj->vec.size(); + }); // class_ OK // classh FAIL diff --git a/tests/test_mi_debug.py b/tests/test_mi_debug.py index d2bbe26f2d..c3ae5eddbe 100644 --- a/tests/test_mi_debug.py +++ b/tests/test_mi_debug.py @@ -2,15 +2,15 @@ def test_get_vec_size_raw_ptr_base0(): - obj = m.make_derived_as_base0() + obj = m.make_derived_as_base0_raw_ptr() assert m.get_vec_size_raw_ptr_base0(obj) == 5 def test_get_vec_size_raw_ptr_derived(): - obj = m.make_derived_as_base0() + obj = m.make_derived_as_base0_raw_ptr() assert m.get_vec_size_raw_ptr_derived(obj) == 5 def test_get_vec_size_shared_ptr_derived(): - obj = m.make_derived_as_base0() + obj = m.make_derived_as_base0_raw_ptr() assert m.get_vec_size_shared_ptr_derived(obj) == 5 From 4e5ee456f4a9c54bc8a1d90b08cf6fbec9242c71 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 2 Dec 2022 17:35:08 -0800 Subject: [PATCH 05/15] WIP --- tests/test_mi_debug.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_mi_debug.py b/tests/test_mi_debug.py index c3ae5eddbe..901f1864d7 100644 --- a/tests/test_mi_debug.py +++ b/tests/test_mi_debug.py @@ -7,7 +7,10 @@ def test_get_vec_size_raw_ptr_base0(): def test_get_vec_size_raw_ptr_derived(): + obj = m.make_derived_as_base0() + print("\nLOOOK", obj) obj = m.make_derived_as_base0_raw_ptr() + print("\nLOOOK", obj) assert m.get_vec_size_raw_ptr_derived(obj) == 5 From e313d86e8fe87ae03d340611e11747a765477bf2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 2 Dec 2022 22:48:54 -0800 Subject: [PATCH 06/15] Debug LOOOK & one-line bug fix: ```diff - auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(src); + auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(std::shared_ptr(src, const_cast(st.first))); ``` --- include/pybind11/detail/smart_holder_poc.h | 5 +++++ include/pybind11/detail/smart_holder_type_casters.h | 9 ++++++++- tests/test_mi_debug.py | 5 ++++- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index de92f4156f..774d2654b2 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -220,6 +220,7 @@ struct smart_holder { } static smart_holder from_raw_ptr_unowned(void *raw_ptr) { + printf("\nLOOOK %s:%d\n", __FILE__, __LINE__); fflush(stdout); smart_holder hld; hld.vptr.reset(raw_ptr, [](void *) {}); hld.vptr_is_using_noop_deleter = true; @@ -250,6 +251,7 @@ struct smart_holder { template static smart_holder from_raw_ptr_take_ownership(T *raw_ptr, bool void_cast_raw_ptr = false) { + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) raw_ptr, __FILE__, __LINE__); fflush(stdout); ensure_pointee_is_destructible("from_raw_ptr_take_ownership"); smart_holder hld; auto gd = make_guarded_builtin_delete(true); @@ -302,6 +304,7 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, bool void_cast_raw_ptr = false) { + printf("\nLOOOK %s:%d\n", __FILE__, __LINE__); fflush(stdout); smart_holder hld; hld.rtti_uqp_del = &typeid(D); hld.vptr_is_using_builtin_delete = is_std_default_delete(*hld.rtti_uqp_del); @@ -333,6 +336,8 @@ struct smart_holder { template static smart_holder from_shared_ptr(std::shared_ptr shd_ptr) { + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) shd_ptr.get(), __FILE__, __LINE__); fflush(stdout); + // long *BAD = nullptr; *BAD = 101; smart_holder hld; hld.vptr = std::static_pointer_cast(shd_ptr); hld.vptr_is_external_shared_ptr = true; diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 9f95e8c72b..b070103bef 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -356,6 +356,8 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag auto *holder_void_ptr = const_cast(holder_const_void_ptr); auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(WrappedType))); + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) v_h.value_ptr(), __FILE__, __LINE__); fflush(stdout); + // long *BAD = nullptr; *BAD = 101; if (!v_h.instance_registered()) { register_instance(inst, v_h.value_ptr(), v_h.type); v_h.set_instance_registered(); @@ -650,6 +652,7 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, } static handle cast(T const *src, return_value_policy policy, handle parent) { + printf("\nLOOOK [%lu] IsBase0Still %s:%d\n", (std::size_t) src, __FILE__, __LINE__); fflush(stdout); auto st = type_caster_base::src_and_type(src); if (policy == return_value_policy::_clif_automatic) { policy = return_value_policy::copy; @@ -702,6 +705,7 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, void *(*copy_constructor)(const void *), void *(*move_constructor)(const void *), const void *existing_holder = nullptr) { + printf("\nLOOOK [%lu] IsDerivedAlready %s:%d\n", (std::size_t) _src, __FILE__, __LINE__); fflush(stdout); if (!tinfo) { // no type info: error will be set already return handle(); } @@ -794,6 +798,7 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l static constexpr auto name = const_name(); static handle cast(const std::shared_ptr &src, return_value_policy policy, handle parent) { + printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) src.get(), __FILE__, __LINE__); fflush(stdout); switch (policy) { case return_value_policy::automatic: case return_value_policy::automatic_reference: @@ -835,8 +840,10 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l inst_raw_ptr->owned = true; void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr(); valueptr = src_raw_void_ptr; + printf("\nLOOOK [%lu] IsBase0 %s:%d\n", (std::size_t) valueptr, __FILE__, __LINE__); fflush(stdout); + printf("\nLOOOK [%lu] IsDerived %s:%d\n", (std::size_t) st.first, __FILE__, __LINE__); fflush(stdout); - auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(src); + auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(std::shared_ptr(src, const_cast(st.first))); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); if (policy == return_value_policy::reference_internal) { diff --git a/tests/test_mi_debug.py b/tests/test_mi_debug.py index 901f1864d7..218aea34f1 100644 --- a/tests/test_mi_debug.py +++ b/tests/test_mi_debug.py @@ -6,9 +6,12 @@ def test_get_vec_size_raw_ptr_base0(): assert m.get_vec_size_raw_ptr_base0(obj) == 5 -def test_get_vec_size_raw_ptr_derived(): +def test_get_vec_size_raw_ptr_derived_from_shared(): obj = m.make_derived_as_base0() print("\nLOOOK", obj) + assert m.get_vec_size_raw_ptr_derived(obj) == 5 + +def test_get_vec_size_raw_ptr_derived(): obj = m.make_derived_as_base0_raw_ptr() print("\nLOOOK", obj) assert m.get_vec_size_raw_ptr_derived(obj) == 5 From 7a5ea90832c631a9706e8f6a162a36d8fb4370fb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 2 Dec 2022 22:58:37 -0800 Subject: [PATCH 07/15] Remove all print LOOOK and clang-format the fix. --- include/pybind11/detail/smart_holder_poc.h | 5 ----- .../pybind11/detail/smart_holder_type_casters.h | 10 ++-------- tests/test_mi_debug.cpp | 14 +------------- tests/test_mi_debug.py | 3 +-- 4 files changed, 4 insertions(+), 28 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 774d2654b2..de92f4156f 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -220,7 +220,6 @@ struct smart_holder { } static smart_holder from_raw_ptr_unowned(void *raw_ptr) { - printf("\nLOOOK %s:%d\n", __FILE__, __LINE__); fflush(stdout); smart_holder hld; hld.vptr.reset(raw_ptr, [](void *) {}); hld.vptr_is_using_noop_deleter = true; @@ -251,7 +250,6 @@ struct smart_holder { template static smart_holder from_raw_ptr_take_ownership(T *raw_ptr, bool void_cast_raw_ptr = false) { - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) raw_ptr, __FILE__, __LINE__); fflush(stdout); ensure_pointee_is_destructible("from_raw_ptr_take_ownership"); smart_holder hld; auto gd = make_guarded_builtin_delete(true); @@ -304,7 +302,6 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, bool void_cast_raw_ptr = false) { - printf("\nLOOOK %s:%d\n", __FILE__, __LINE__); fflush(stdout); smart_holder hld; hld.rtti_uqp_del = &typeid(D); hld.vptr_is_using_builtin_delete = is_std_default_delete(*hld.rtti_uqp_del); @@ -336,8 +333,6 @@ struct smart_holder { template static smart_holder from_shared_ptr(std::shared_ptr shd_ptr) { - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) shd_ptr.get(), __FILE__, __LINE__); fflush(stdout); - // long *BAD = nullptr; *BAD = 101; smart_holder hld; hld.vptr = std::static_pointer_cast(shd_ptr); hld.vptr_is_external_shared_ptr = true; diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index b070103bef..56155add86 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -356,8 +356,6 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag auto *holder_void_ptr = const_cast(holder_const_void_ptr); auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(WrappedType))); - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) v_h.value_ptr(), __FILE__, __LINE__); fflush(stdout); - // long *BAD = nullptr; *BAD = 101; if (!v_h.instance_registered()) { register_instance(inst, v_h.value_ptr(), v_h.type); v_h.set_instance_registered(); @@ -652,7 +650,6 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, } static handle cast(T const *src, return_value_policy policy, handle parent) { - printf("\nLOOOK [%lu] IsBase0Still %s:%d\n", (std::size_t) src, __FILE__, __LINE__); fflush(stdout); auto st = type_caster_base::src_and_type(src); if (policy == return_value_policy::_clif_automatic) { policy = return_value_policy::copy; @@ -705,7 +702,6 @@ struct smart_holder_type_caster : smart_holder_type_caster_load, void *(*copy_constructor)(const void *), void *(*move_constructor)(const void *), const void *existing_holder = nullptr) { - printf("\nLOOOK [%lu] IsDerivedAlready %s:%d\n", (std::size_t) _src, __FILE__, __LINE__); fflush(stdout); if (!tinfo) { // no type info: error will be set already return handle(); } @@ -798,7 +794,6 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l static constexpr auto name = const_name(); static handle cast(const std::shared_ptr &src, return_value_policy policy, handle parent) { - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) src.get(), __FILE__, __LINE__); fflush(stdout); switch (policy) { case return_value_policy::automatic: case return_value_policy::automatic_reference: @@ -840,10 +835,9 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l inst_raw_ptr->owned = true; void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr(); valueptr = src_raw_void_ptr; - printf("\nLOOOK [%lu] IsBase0 %s:%d\n", (std::size_t) valueptr, __FILE__, __LINE__); fflush(stdout); - printf("\nLOOOK [%lu] IsDerived %s:%d\n", (std::size_t) st.first, __FILE__, __LINE__); fflush(stdout); - auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(std::shared_ptr(src, const_cast(st.first))); + auto smhldr = pybindit::memory::smart_holder::from_shared_ptr( + std::shared_ptr(src, const_cast(st.first))); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); if (policy == return_value_policy::reference_internal) { diff --git a/tests/test_mi_debug.cpp b/tests/test_mi_debug.cpp index 7b57c9d7c0..759524fb3b 100644 --- a/tests/test_mi_debug.cpp +++ b/tests/test_mi_debug.cpp @@ -56,11 +56,7 @@ TEST_SUBMODULE(mi_debug, m) { m.def("make_derived_as_base0", []() -> std::shared_ptr { auto ret_der = std::make_shared(); - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) ret_der.get(), __FILE__, __LINE__); - fflush(stdout); auto ret = std::dynamic_pointer_cast(ret_der); - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) ret.get(), __FILE__, __LINE__); - fflush(stdout); return ret; }); @@ -68,11 +64,7 @@ TEST_SUBMODULE(mi_debug, m) { "make_derived_as_base0_raw_ptr", []() { auto ret_der = new Derived{}; - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) ret_der, __FILE__, __LINE__); - fflush(stdout); auto ret = dynamic_cast(ret_der); - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) ret, __FILE__, __LINE__); - fflush(stdout); return ret; }, py::return_value_policy::take_ownership); @@ -89,11 +81,7 @@ TEST_SUBMODULE(mi_debug, m) { // class_ OK // classh FAIL - m.def("get_vec_size_raw_ptr_derived", [](const Derived *obj) { - printf("\nLOOOK [%lu] %s:%d\n", (std::size_t) obj, __FILE__, __LINE__); - fflush(stdout); - return obj->vec.size(); - }); + m.def("get_vec_size_raw_ptr_derived", [](const Derived *obj) { return obj->vec.size(); }); // class_ OK // classh FAIL diff --git a/tests/test_mi_debug.py b/tests/test_mi_debug.py index 218aea34f1..15db3c32bb 100644 --- a/tests/test_mi_debug.py +++ b/tests/test_mi_debug.py @@ -8,12 +8,11 @@ def test_get_vec_size_raw_ptr_base0(): def test_get_vec_size_raw_ptr_derived_from_shared(): obj = m.make_derived_as_base0() - print("\nLOOOK", obj) assert m.get_vec_size_raw_ptr_derived(obj) == 5 + def test_get_vec_size_raw_ptr_derived(): obj = m.make_derived_as_base0_raw_ptr() - print("\nLOOOK", obj) assert m.get_vec_size_raw_ptr_derived(obj) == 5 From 8aa790b7ff0c6af17173832645b0bd4abf61fbed Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 4 Dec 2022 03:30:31 -0800 Subject: [PATCH 08/15] Resolve clang-tidy errors. --- tests/test_mi_debug.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_mi_debug.cpp b/tests/test_mi_debug.cpp index 759524fb3b..857f617deb 100644 --- a/tests/test_mi_debug.cpp +++ b/tests/test_mi_debug.cpp @@ -63,8 +63,8 @@ TEST_SUBMODULE(mi_debug, m) { m.def( "make_derived_as_base0_raw_ptr", []() { - auto ret_der = new Derived{}; - auto ret = dynamic_cast(ret_der); + auto *ret_der = new Derived{}; + auto *ret = dynamic_cast(ret_der); return ret; }, py::return_value_policy::take_ownership); @@ -72,7 +72,7 @@ TEST_SUBMODULE(mi_debug, m) { // class_ OK // classh FAIL m.def("get_vec_size_raw_ptr_base0", [](const Base0 *obj) -> std::size_t { - auto obj_der = dynamic_cast(obj); + const auto *obj_der = dynamic_cast(obj); if (obj_der == nullptr) { return 0; } From f96201a6c4e46bfb2e5cb22e7fbdf72beff69e5c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Dec 2022 14:59:23 -0800 Subject: [PATCH 09/15] Systematic test matrix. --- tests/test_mi_debug.cpp | 62 ++++++++++++++++++++--------------------- tests/test_mi_debug.py | 57 +++++++++++++++++++++++++------------ 2 files changed, 69 insertions(+), 50 deletions(-) diff --git a/tests/test_mi_debug.cpp b/tests/test_mi_debug.cpp index 857f617deb..354a8b0b6b 100644 --- a/tests/test_mi_debug.cpp +++ b/tests/test_mi_debug.cpp @@ -1,10 +1,5 @@ #include - -#define USE_SH - -#if defined(USE_SH) -# include -#endif +#include #include "pybind11_tests.h" @@ -35,33 +30,19 @@ struct Derived : Base1, Base0 { } // namespace test_mi_debug -#if defined(USE_SH) PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Base0) PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Base1) PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Derived) -#endif TEST_SUBMODULE(mi_debug, m) { using namespace test_mi_debug; -#if defined(USE_SH) py::classh bs0(m, "Base0"); py::classh bs1(m, "Base1"); - py::classh(m, "Derived").def(py::init<>()); -#else - py::class_> bs0(m, "Base0"); - py::class_> bs1(m, "Base1"); - py::class_, Base1, Base0>(m, "Derived").def(py::init<>()); -#endif - - m.def("make_derived_as_base0", []() -> std::shared_ptr { - auto ret_der = std::make_shared(); - auto ret = std::dynamic_pointer_cast(ret_der); - return ret; - }); + py::classh(m, "Derived"); m.def( - "make_derived_as_base0_raw_ptr", + "get_derived_as_base0_raw_ptr", []() { auto *ret_der = new Derived{}; auto *ret = dynamic_cast(ret_der); @@ -69,9 +50,19 @@ TEST_SUBMODULE(mi_debug, m) { }, py::return_value_policy::take_ownership); - // class_ OK - // classh FAIL - m.def("get_vec_size_raw_ptr_base0", [](const Base0 *obj) -> std::size_t { + m.def("get_derived_as_base0_shared_ptr", []() -> std::shared_ptr { + auto ret_der = std::make_shared(); + auto ret = std::dynamic_pointer_cast(ret_der); + return ret; + }); + + m.def("get_derived_as_base0_unique_ptr", []() -> std::unique_ptr { + auto ret_der = std::unique_ptr(new Derived{}); + auto ret = std::unique_ptr(std::move(ret_der)); + return ret; + }); + + m.def("vec_size_base0_raw_ptr", [](const Base0 *obj) -> std::size_t { const auto *obj_der = dynamic_cast(obj); if (obj_der == nullptr) { return 0; @@ -79,12 +70,19 @@ TEST_SUBMODULE(mi_debug, m) { return obj_der->vec.size(); }); - // class_ OK - // classh FAIL - m.def("get_vec_size_raw_ptr_derived", [](const Derived *obj) { return obj->vec.size(); }); + m.def("vec_size_base0_shared_ptr", [](const std::shared_ptr &obj) -> std::size_t { + const auto obj_der = std::dynamic_pointer_cast(obj); + if (!obj_der) { + return 0; + } + return obj_der->vec.size(); + }); - // class_ OK - // classh FAIL - m.def("get_vec_size_shared_ptr_derived", - [](const std::shared_ptr &obj) { return obj->vec.size(); }); + m.def("vec_size_base0_unique_ptr", [](std::unique_ptr obj) -> std::size_t { + const auto *obj_der = dynamic_cast(obj.get()); + if (obj_der == nullptr) { + return 0; + } + return obj_der->vec.size(); + }); } diff --git a/tests/test_mi_debug.py b/tests/test_mi_debug.py index 15db3c32bb..d09bf30275 100644 --- a/tests/test_mi_debug.py +++ b/tests/test_mi_debug.py @@ -1,21 +1,42 @@ -from pybind11_tests import mi_debug as m - - -def test_get_vec_size_raw_ptr_base0(): - obj = m.make_derived_as_base0_raw_ptr() - assert m.get_vec_size_raw_ptr_base0(obj) == 5 - +import pytest -def test_get_vec_size_raw_ptr_derived_from_shared(): - obj = m.make_derived_as_base0() - assert m.get_vec_size_raw_ptr_derived(obj) == 5 - - -def test_get_vec_size_raw_ptr_derived(): - obj = m.make_derived_as_base0_raw_ptr() - assert m.get_vec_size_raw_ptr_derived(obj) == 5 +from pybind11_tests import mi_debug as m -def test_get_vec_size_shared_ptr_derived(): - obj = m.make_derived_as_base0_raw_ptr() - assert m.get_vec_size_shared_ptr_derived(obj) == 5 +@pytest.mark.parametrize( + "vec_size_fn", + [ + m.vec_size_base0_raw_ptr, + m.vec_size_base0_shared_ptr, + m.vec_size_base0_unique_ptr, + ], +) +@pytest.mark.parametrize( + "get_fn", + [ + m.get_derived_as_base0_raw_ptr, + m.get_derived_as_base0_shared_ptr, + # *** FAILS *** m.get_derived_as_base0_unique_ptr, + ], +) +def test_get_vec_size(get_fn, vec_size_fn): + obj = get_fn() + if ( + get_fn is m.get_derived_as_base0_shared_ptr + and vec_size_fn is m.vec_size_base0_unique_ptr + ): + with pytest.raises(ValueError) as exc_info: + vec_size_fn(obj) + assert ( + str(exc_info.value) + == "Cannot disown external shared_ptr (loaded_as_unique_ptr)." + ) + else: + assert vec_size_fn(obj) == 5 + if vec_size_fn is m.vec_size_base0_unique_ptr: + with pytest.raises(ValueError) as exc_info: + vec_size_fn(obj) + assert ( + str(exc_info.value) + == "Missing value for wrapped C++ type: Python instance was disowned." + ) From 8f18d7af28bcabf77a1e29f02a7e2c07005e6948 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Dec 2022 22:35:32 -0800 Subject: [PATCH 10/15] Bug fix in `smart_holder_type_caster>::cast()` --- include/pybind11/detail/smart_holder_poc.h | 6 +++--- .../detail/smart_holder_type_casters.h | 21 ++++++++++++------- tests/test_mi_debug.py | 2 +- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index de92f4156f..e169f2f26b 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -301,7 +301,7 @@ struct smart_holder { template static smart_holder from_unique_ptr(std::unique_ptr &&unq_ptr, - bool void_cast_raw_ptr = false) { + void *void_ptr = nullptr) { smart_holder hld; hld.rtti_uqp_del = &typeid(D); hld.vptr_is_using_builtin_delete = is_std_default_delete(*hld.rtti_uqp_del); @@ -311,8 +311,8 @@ struct smart_holder { } else { gd = make_guarded_custom_deleter(true); } - if (void_cast_raw_ptr) { - hld.vptr.reset(static_cast(unq_ptr.get()), std::move(gd)); + if (void_ptr != nullptr) { + hld.vptr.reset(void_ptr, std::move(gd)); } else { hld.vptr.reset(unq_ptr.get(), std::move(gd)); } diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 56155add86..b3289773a7 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -386,8 +386,8 @@ 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, bool void_cast_raw_ptr) { - return pybindit::memory::smart_holder::from_unique_ptr(std::move(unq_ptr), - void_cast_raw_ptr); + void *void_ptr = void_cast_raw_ptr ? static_cast(unq_ptr.get()) : nullptr; + return pybindit::memory::smart_holder::from_unique_ptr(std::move(unq_ptr), void_ptr); } template @@ -891,17 +891,16 @@ struct smart_holder_type_caster> : smart_holder_type_caste return none().release(); } - auto src_raw_ptr = src.get(); - auto st = type_caster_base::src_and_type(src_raw_ptr); + auto st = type_caster_base::src_and_type(src.get()); if (st.second == nullptr) { return handle(); // no type info: error will be set already } - void *src_raw_void_ptr = static_cast(src_raw_ptr); + void *src_raw_void_ptr = const_cast(st.first); const detail::type_info *tinfo = st.second; if (handle existing_inst = find_registered_python_instance(src_raw_void_ptr, tinfo)) { auto *self_life_support - = dynamic_raw_ptr_cast_if_possible(src_raw_ptr); + = dynamic_raw_ptr_cast_if_possible(src.get()); if (self_life_support != nullptr) { value_and_holder &v_h = self_life_support->v_h; if (v_h.inst != nullptr && v_h.vh != nullptr) { @@ -927,8 +926,14 @@ 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), - /*void_cast_raw_ptr*/ false); + if (static_cast(src.get()) == src_raw_void_ptr) { + // This is a multiple-inheritance situation that is incompatible with the current + // shared_from_this handling (see PR #3023). + // SMART_HOLDER_WIP: IMPROVABLE: Is there a better solution? + src_raw_void_ptr = nullptr; + } + auto smhldr + = pybindit::memory::smart_holder::from_unique_ptr(std::move(src), src_raw_void_ptr); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); if (policy == return_value_policy::reference_internal) { diff --git a/tests/test_mi_debug.py b/tests/test_mi_debug.py index d09bf30275..06e96c5eaf 100644 --- a/tests/test_mi_debug.py +++ b/tests/test_mi_debug.py @@ -16,7 +16,7 @@ [ m.get_derived_as_base0_raw_ptr, m.get_derived_as_base0_shared_ptr, - # *** FAILS *** m.get_derived_as_base0_unique_ptr, + m.get_derived_as_base0_unique_ptr, ], ) def test_get_vec_size(get_fn, vec_size_fn): From 3c45051450b80b9d1d196f78067cf373b1cbcf91 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 10 Dec 2022 10:45:17 -0800 Subject: [PATCH 11/15] Rename: test_mi_debug -> test_class_sh_mi_thunks --- tests/CMakeLists.txt | 2 +- ...st_mi_debug.cpp => test_class_sh_mi_thunks.cpp} | 14 +++++++------- ...test_mi_debug.py => test_class_sh_mi_thunks.py} | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) rename tests/{test_mi_debug.cpp => test_class_sh_mi_thunks.cpp} (85%) rename tests/{test_mi_debug.py => test_class_sh_mi_thunks.py} (95%) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index fa185a2f8e..e6970739ff 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -127,6 +127,7 @@ set(PYBIND11_TEST_FILES test_class_sh_disowning_mi test_class_sh_factory_constructors test_class_sh_inheritance + test_class_sh_mi_thunks test_class_sh_module_local.py test_class_sh_property test_class_sh_shared_ptr_copy_move @@ -158,7 +159,6 @@ set(PYBIND11_TEST_FILES test_kwargs_and_defaults test_local_bindings test_methods_and_attributes - test_mi_debug test_modules test_multiple_inheritance test_numpy_array diff --git a/tests/test_mi_debug.cpp b/tests/test_class_sh_mi_thunks.cpp similarity index 85% rename from tests/test_mi_debug.cpp rename to tests/test_class_sh_mi_thunks.cpp index 354a8b0b6b..5d256a3263 100644 --- a/tests/test_mi_debug.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -7,7 +7,7 @@ #include #include -namespace test_mi_debug { +namespace test_class_sh_mi_thunks { struct Base0 { virtual ~Base0() = default; @@ -28,14 +28,14 @@ struct Derived : Base1, Base0 { Derived(const Derived &) = delete; }; -} // namespace test_mi_debug +} // namespace test_class_sh_mi_thunks -PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Base0) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Base1) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_mi_debug::Derived) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_mi_thunks::Base0) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_mi_thunks::Base1) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_mi_thunks::Derived) -TEST_SUBMODULE(mi_debug, m) { - using namespace test_mi_debug; +TEST_SUBMODULE(class_sh_mi_thunks, m) { + using namespace test_class_sh_mi_thunks; py::classh bs0(m, "Base0"); py::classh bs1(m, "Base1"); diff --git a/tests/test_mi_debug.py b/tests/test_class_sh_mi_thunks.py similarity index 95% rename from tests/test_mi_debug.py rename to tests/test_class_sh_mi_thunks.py index 06e96c5eaf..13a77dc8c1 100644 --- a/tests/test_mi_debug.py +++ b/tests/test_class_sh_mi_thunks.py @@ -1,6 +1,6 @@ import pytest -from pybind11_tests import mi_debug as m +from pybind11_tests import class_sh_mi_thunks as m @pytest.mark.parametrize( From 73a905235c72a5490022e2f6c3929fe9bc9bab54 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 10 Dec 2022 14:02:54 -0800 Subject: [PATCH 12/15] Add `test_ptrdiff_derived_base0()` --- tests/test_class_sh_mi_thunks.cpp | 7 +++++++ tests/test_class_sh_mi_thunks.py | 9 +++++++++ 2 files changed, 16 insertions(+) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 5d256a3263..34ac1835fb 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -37,6 +37,13 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_mi_thunks::Derived) TEST_SUBMODULE(class_sh_mi_thunks, m) { using namespace test_class_sh_mi_thunks; + m.def("ptrdiff_derived_base0", []() { + auto drvd = std::unique_ptr(new Derived{}); + auto base0 = dynamic_cast(drvd.get()); + return std::ptrdiff_t(reinterpret_cast(drvd.get()) + - reinterpret_cast(base0)); + }); + py::classh bs0(m, "Base0"); py::classh bs1(m, "Base1"); py::classh(m, "Derived"); diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index 13a77dc8c1..f11e94aa2a 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -3,6 +3,15 @@ from pybind11_tests import class_sh_mi_thunks as m +def test_ptrdiff_derived_base0(): + ptrdiff = m.ptrdiff_derived_base0() + # A failure here does not (necessarily) mean that there is a bug, but that + # test_class_sh_mi_thunks is not exercising what it is supposed to. + # If this ever fails on some platforms: use pytest.skip() + # If this ever fails on all platforms: don't know, seems extremely unlikely. + assert ptrdiff != 0 + + @pytest.mark.parametrize( "vec_size_fn", [ From f31dafae96d48cc3a5d856657fc1e21a4106a9c4 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 10 Dec 2022 14:23:42 -0800 Subject: [PATCH 13/15] Miscellaneous polishing (naming, comments). No functional changes. --- tests/test_class_sh_mi_thunks.cpp | 45 +++++++++++++++++-------------- tests/test_class_sh_mi_thunks.py | 12 ++++----- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index 34ac1835fb..b2e6e661a2 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -9,6 +9,10 @@ namespace test_class_sh_mi_thunks { +// For general background: https://shaharmike.com/cpp/vtable-part2/ +// C++ vtables - Part 2 - Multiple Inheritance +// ... the compiler creates a 'thunk' method that corrects `this` ... + struct Base0 { virtual ~Base0() = default; Base0() = default; @@ -17,6 +21,7 @@ struct Base0 { struct Base1 { virtual ~Base1() = default; + // Using `vector` here because it is known to make this test very sensitive to bugs. std::vector vec = {1, 2, 3, 4, 5}; Base1() = default; Base1(const Base1 &) = delete; @@ -37,42 +42,42 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_mi_thunks::Derived) TEST_SUBMODULE(class_sh_mi_thunks, m) { using namespace test_class_sh_mi_thunks; - m.def("ptrdiff_derived_base0", []() { - auto drvd = std::unique_ptr(new Derived{}); + m.def("ptrdiff_drvd_base0", []() { + auto drvd = std::unique_ptr(new Derived); auto base0 = dynamic_cast(drvd.get()); return std::ptrdiff_t(reinterpret_cast(drvd.get()) - reinterpret_cast(base0)); }); - py::classh bs0(m, "Base0"); - py::classh bs1(m, "Base1"); + py::classh(m, "Base0"); + py::classh(m, "Base1"); py::classh(m, "Derived"); m.def( - "get_derived_as_base0_raw_ptr", + "get_drvd_as_base0_raw_ptr", []() { - auto *ret_der = new Derived{}; - auto *ret = dynamic_cast(ret_der); - return ret; + auto *drvd = new Derived; + auto *base0 = dynamic_cast(drvd); + return base0; }, py::return_value_policy::take_ownership); - m.def("get_derived_as_base0_shared_ptr", []() -> std::shared_ptr { - auto ret_der = std::make_shared(); - auto ret = std::dynamic_pointer_cast(ret_der); - return ret; + m.def("get_drvd_as_base0_shared_ptr", []() { + auto drvd = std::make_shared(); + auto base0 = std::dynamic_pointer_cast(drvd); + return base0; }); - m.def("get_derived_as_base0_unique_ptr", []() -> std::unique_ptr { - auto ret_der = std::unique_ptr(new Derived{}); - auto ret = std::unique_ptr(std::move(ret_der)); - return ret; + m.def("get_drvd_as_base0_unique_ptr", []() { + auto drvd = std::unique_ptr(new Derived); + auto base0 = std::unique_ptr(std::move(drvd)); + return base0; }); - m.def("vec_size_base0_raw_ptr", [](const Base0 *obj) -> std::size_t { + m.def("vec_size_base0_raw_ptr", [](const Base0 *obj) { const auto *obj_der = dynamic_cast(obj); if (obj_der == nullptr) { - return 0; + return std::size_t(0); } return obj_der->vec.size(); }); @@ -80,7 +85,7 @@ TEST_SUBMODULE(class_sh_mi_thunks, m) { m.def("vec_size_base0_shared_ptr", [](const std::shared_ptr &obj) -> std::size_t { const auto obj_der = std::dynamic_pointer_cast(obj); if (!obj_der) { - return 0; + return std::size_t(0); } return obj_der->vec.size(); }); @@ -88,7 +93,7 @@ TEST_SUBMODULE(class_sh_mi_thunks, m) { m.def("vec_size_base0_unique_ptr", [](std::unique_ptr obj) -> std::size_t { const auto *obj_der = dynamic_cast(obj.get()); if (obj_der == nullptr) { - return 0; + return std::size_t(0); } return obj_der->vec.size(); }); diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index f11e94aa2a..9fec3c4b6f 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -3,8 +3,8 @@ from pybind11_tests import class_sh_mi_thunks as m -def test_ptrdiff_derived_base0(): - ptrdiff = m.ptrdiff_derived_base0() +def test_ptrdiff_drvd_base0(): + ptrdiff = m.ptrdiff_drvd_base0() # A failure here does not (necessarily) mean that there is a bug, but that # test_class_sh_mi_thunks is not exercising what it is supposed to. # If this ever fails on some platforms: use pytest.skip() @@ -23,15 +23,15 @@ def test_ptrdiff_derived_base0(): @pytest.mark.parametrize( "get_fn", [ - m.get_derived_as_base0_raw_ptr, - m.get_derived_as_base0_shared_ptr, - m.get_derived_as_base0_unique_ptr, + m.get_drvd_as_base0_raw_ptr, + m.get_drvd_as_base0_shared_ptr, + m.get_drvd_as_base0_unique_ptr, ], ) def test_get_vec_size(get_fn, vec_size_fn): obj = get_fn() if ( - get_fn is m.get_derived_as_base0_shared_ptr + get_fn is m.get_drvd_as_base0_shared_ptr and vec_size_fn is m.vec_size_base0_unique_ptr ): with pytest.raises(ValueError) as exc_info: From 6512c02ed2aef16cd34ecd45e00a9b3a33acbd25 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 10 Dec 2022 14:42:56 -0800 Subject: [PATCH 14/15] Improve test_class_sh_mi_thunks.py implementation. No change in test coverage. --- tests/test_class_sh_mi_thunks.py | 47 ++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index 9fec3c4b6f..7d6e3849a9 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -17,7 +17,6 @@ def test_ptrdiff_drvd_base0(): [ m.vec_size_base0_raw_ptr, m.vec_size_base0_shared_ptr, - m.vec_size_base0_unique_ptr, ], ) @pytest.mark.parametrize( @@ -28,24 +27,30 @@ def test_ptrdiff_drvd_base0(): m.get_drvd_as_base0_unique_ptr, ], ) -def test_get_vec_size(get_fn, vec_size_fn): +def test_get_vec_size_raw_shared(get_fn, vec_size_fn): obj = get_fn() - if ( - get_fn is m.get_drvd_as_base0_shared_ptr - and vec_size_fn is m.vec_size_base0_unique_ptr - ): - with pytest.raises(ValueError) as exc_info: - vec_size_fn(obj) - assert ( - str(exc_info.value) - == "Cannot disown external shared_ptr (loaded_as_unique_ptr)." - ) - else: - assert vec_size_fn(obj) == 5 - if vec_size_fn is m.vec_size_base0_unique_ptr: - with pytest.raises(ValueError) as exc_info: - vec_size_fn(obj) - assert ( - str(exc_info.value) - == "Missing value for wrapped C++ type: Python instance was disowned." - ) + assert vec_size_fn(obj) == 5 + + +@pytest.mark.parametrize( + "get_fn", [m.get_drvd_as_base0_raw_ptr, m.get_drvd_as_base0_unique_ptr] +) +def test_get_vec_size_unique(get_fn): + obj = get_fn() + assert m.vec_size_base0_unique_ptr(obj) == 5 + with pytest.raises(ValueError) as exc_info: + m.vec_size_base0_unique_ptr(obj) + assert ( + str(exc_info.value) + == "Missing value for wrapped C++ type: Python instance was disowned." + ) + + +def test_get_shared_vec_size_unique(): + obj = m.get_drvd_as_base0_shared_ptr() + with pytest.raises(ValueError) as exc_info: + m.vec_size_base0_unique_ptr(obj) + assert ( + str(exc_info.value) + == "Cannot disown external shared_ptr (loaded_as_unique_ptr)." + ) From c1b2b6c54cbeae22590ef2368808511c43bf78bd Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 10 Dec 2022 15:06:47 -0800 Subject: [PATCH 15/15] Resolve clang-tidy error. --- tests/test_class_sh_mi_thunks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sh_mi_thunks.cpp b/tests/test_class_sh_mi_thunks.cpp index b2e6e661a2..c4f430e864 100644 --- a/tests/test_class_sh_mi_thunks.cpp +++ b/tests/test_class_sh_mi_thunks.cpp @@ -44,7 +44,7 @@ TEST_SUBMODULE(class_sh_mi_thunks, m) { m.def("ptrdiff_drvd_base0", []() { auto drvd = std::unique_ptr(new Derived); - auto base0 = dynamic_cast(drvd.get()); + auto *base0 = dynamic_cast(drvd.get()); return std::ptrdiff_t(reinterpret_cast(drvd.get()) - reinterpret_cast(base0)); });