From 84033fd66faeb724e7a130f3d8ee5a2a57aed705 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Tue, 31 Oct 2023 19:10:02 -0400 Subject: [PATCH 1/3] Adding some unit tests for std::function deleters. Last unit test shows that std::function doesn't survive a roundtrip. --- tests/pure_cpp/smart_holder_poc_test.cpp | 58 ++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/pure_cpp/smart_holder_poc_test.cpp b/tests/pure_cpp/smart_holder_poc_test.cpp index a10e0ad362..7ba5bad1f6 100644 --- a/tests/pure_cpp/smart_holder_poc_test.cpp +++ b/tests/pure_cpp/smart_holder_poc_test.cpp @@ -276,6 +276,23 @@ TEST_CASE("from_unique_ptr_with_std_function_deleter+as_lvalue_ref", "[S]") { REQUIRE(hld.as_lvalue_ref() == 19); } +TEST_CASE("from_unique_ptr_with_std_function_deleter+as_lvalue_ref+destructor_called", "[S]") { + bool destructor_called = false; + std::unique_ptr> orig_owner( + new int(19), + [&destructor_called](const int *raw_ptr) { destructor_called = true; delete raw_ptr; }); + { + auto hld = smart_holder::from_unique_ptr(std::move(orig_owner)); + REQUIRE(orig_owner.get() == nullptr); + REQUIRE(hld.as_lvalue_ref() == 19); + + // At this point, the destructor shouldn't have been called yet, it is moved to the holder. + REQUIRE(destructor_called == false); + } + // Smart holder goes out of scope, the original destructor should have been called. + REQUIRE(destructor_called); +} + TEST_CASE("from_unique_ptr_with_deleter+as_raw_ptr_release_ownership", "[E]") { std::unique_ptr> orig_owner(new int(19)); auto hld = smart_holder::from_unique_ptr(std::move(orig_owner)); @@ -284,6 +301,26 @@ TEST_CASE("from_unique_ptr_with_deleter+as_raw_ptr_release_ownership", "[E]") { "Cannot disown custom deleter (as_raw_ptr_release_ownership)."); } +TEST_CASE("from_unique_ptr_with_std_function_deleter+as_raw_ptr_release_ownership", "[S]") { + bool destructor_called = false; + std::unique_ptr> orig_owner( + new int(19), + [&destructor_called](const int *raw_ptr) { destructor_called = true; delete raw_ptr; }); + { + auto hld = smart_holder::from_unique_ptr(std::move(orig_owner)); + REQUIRE(orig_owner.get() == nullptr); + + // Destructor shouldn't be called during the moving to the smart holder. + REQUIRE(destructor_called == false); + + REQUIRE_THROWS_WITH(hld.as_raw_ptr_release_ownership(), + "Cannot disown custom deleter (as_raw_ptr_release_ownership)."); + + } + // Smart holder goes out of scope, the original destructor should have been called. + REQUIRE(destructor_called); +} + TEST_CASE("from_unique_ptr_with_deleter+as_unique_ptr", "[E]") { std::unique_ptr> orig_owner(new int(19)); auto hld = smart_holder::from_unique_ptr(std::move(orig_owner)); @@ -302,6 +339,27 @@ TEST_CASE("from_unique_ptr_with_deleter+as_unique_ptr_with_deleter1", "[S]") { REQUIRE(*new_owner == 19); } + +TEST_CASE("from_unique_ptr_std_function_with_deleter+as_unique_ptr_with_std_function_deleter1", "[S]") { + bool destructor_called = false; + using OpaqueDeleter = std::function; + + std::unique_ptr orig_owner( + new int(19), + OpaqueDeleter([&destructor_called](const void *raw_ptr) { destructor_called = true; delete static_cast(raw_ptr); })); + auto hld = smart_holder::from_unique_ptr(std::move(orig_owner)); + REQUIRE(orig_owner.get() == nullptr); + std::unique_ptr new_owner = hld.as_unique_ptr(); + REQUIRE(!hld.has_pointee()); + REQUIRE(*new_owner == 19); + + REQUIRE(destructor_called == false); + new_owner.reset(); + REQUIRE(destructor_called); + +} + + TEST_CASE("from_unique_ptr_with_deleter+as_unique_ptr_with_deleter2", "[E]") { std::unique_ptr> orig_owner(new int(19)); auto hld = smart_holder::from_unique_ptr(std::move(orig_owner)); From 1ce70539e9a5bd696e6b561f93ecebc8cb5348c9 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Tue, 31 Oct 2023 20:19:08 -0400 Subject: [PATCH 2/3] Roundtrip of the custom deleter from unique_ptr to holder to unique_ptr. --- include/pybind11/detail/smart_holder_poc.h | 43 ++++++++++++++++++++++ tests/pure_cpp/CMakeLists.txt | 1 + tests/pure_cpp/smart_holder_poc_test.cpp | 1 - 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 65b0581617..321d91b1ae 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -68,6 +68,22 @@ static constexpr bool type_has_shared_from_this(const std::enable_shared_from_th return true; } +namespace detail { +template struct delete_assigner { + static void assign(D&, bool, std::function&, void(*)(void*)&){} +}; + +template struct delete_assigner { + static void assign(D& deleter, bool use_del_fun, std::function& del_fun, void(*del_ptr)(void*)&){ + if (use_del_fun) { + deleter = std::move(del_fun); + } else { + deleter = del_ptr; + } + } +}; +} + struct guarded_delete { std::weak_ptr released_ptr; // Trick to keep the smart_holder memory footprint small. std::function del_fun; // Rare case. @@ -88,6 +104,15 @@ struct guarded_delete { } } } + + template + bool extract_deleter(D& deleter) { + if (armed_flag) { + detail::delete_assigner::value>::assign(deleter, use_del_fun, del_fun, del_ptr); + return true; + } + return false; + } }; template ::value, int>::type = 0> @@ -340,7 +365,25 @@ struct smart_holder { ensure_compatible_rtti_uqp_del(context); ensure_use_count_1(context); T *raw_ptr = as_raw_ptr_unowned(); + + // extract the custom deleter from the holder; + D extracted_deleter; + + auto *gd = std::get_deleter(vptr); + + // There's no way to remove the deleter from a shared pointer, reach in and extract the + // deletion action before the shared pointer destructor invokes it. + bool found_deleter = false; + if (gd) { + found_deleter = gd->extract_deleter(extracted_deleter); + } + release_ownership(); + + if (found_deleter) { + return std::unique_ptr(raw_ptr, std::move(extracted_deleter)); + } + // No special destructor. return std::unique_ptr(raw_ptr); } diff --git a/tests/pure_cpp/CMakeLists.txt b/tests/pure_cpp/CMakeLists.txt index 17be74b7f0..164ea96986 100644 --- a/tests/pure_cpp/CMakeLists.txt +++ b/tests/pure_cpp/CMakeLists.txt @@ -9,6 +9,7 @@ else() endif() add_executable(smart_holder_poc_test smart_holder_poc_test.cpp) +target_compile_options(smart_holder_poc_test PRIVATE "-g") pybind11_enable_warnings(smart_holder_poc_test) target_link_libraries(smart_holder_poc_test PRIVATE pybind11::headers Catch2::Catch2) diff --git a/tests/pure_cpp/smart_holder_poc_test.cpp b/tests/pure_cpp/smart_holder_poc_test.cpp index 7ba5bad1f6..26eabb4b0c 100644 --- a/tests/pure_cpp/smart_holder_poc_test.cpp +++ b/tests/pure_cpp/smart_holder_poc_test.cpp @@ -356,7 +356,6 @@ TEST_CASE("from_unique_ptr_std_function_with_deleter+as_unique_ptr_with_std_func REQUIRE(destructor_called == false); new_owner.reset(); REQUIRE(destructor_called); - } From d9f571ca67bf00774625022a5864165fd4c61db6 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Tue, 31 Oct 2023 20:43:54 -0400 Subject: [PATCH 3/3] Bit of cleanup, comments. --- include/pybind11/detail/smart_holder_poc.h | 29 ++++++++++++---------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/include/pybind11/detail/smart_holder_poc.h b/include/pybind11/detail/smart_holder_poc.h index 321d91b1ae..6eb3662aaf 100644 --- a/include/pybind11/detail/smart_holder_poc.h +++ b/include/pybind11/detail/smart_holder_poc.h @@ -70,16 +70,17 @@ static constexpr bool type_has_shared_from_this(const std::enable_shared_from_th namespace detail { template struct delete_assigner { - static void assign(D&, bool, std::function&, void(*)(void*)&){} + static bool assign(D&, bool, std::function&, void(*)(void*)&){ return false; } }; template struct delete_assigner { - static void assign(D& deleter, bool use_del_fun, std::function& del_fun, void(*del_ptr)(void*)&){ + static bool assign(D& deleter, bool use_del_fun, std::function& del_fun, void(*del_ptr)(void*)&){ if (use_del_fun) { deleter = std::move(del_fun); } else { deleter = del_ptr; } + return true; } }; } @@ -105,11 +106,15 @@ struct guarded_delete { } } - template + template bool extract_deleter(D& deleter) { if (armed_flag) { - detail::delete_assigner::value>::assign(deleter, use_del_fun, del_fun, del_ptr); - return true; + // Just doing a direct assignment here like; 'deleter = del_ptr' doesn't work in case + // the deleter is of a type non-constructible from the custom deleter like + // helpers::functor_other_delete’, so we use a simple trait system to assign if the + // types allow it. + armed_flag = false; + return detail::delete_assigner::value>::assign(deleter, use_del_fun, del_fun, del_ptr); } return false; } @@ -366,23 +371,21 @@ struct smart_holder { ensure_use_count_1(context); T *raw_ptr = as_raw_ptr_unowned(); - // extract the custom deleter from the holder; + // Temporary storage of the deleter function if it exists in the holder, places the + // requirement that the deleter is default constructible... D extracted_deleter; + // Extract the deleter function if it exists. auto *gd = std::get_deleter(vptr); - - // There's no way to remove the deleter from a shared pointer, reach in and extract the - // deletion action before the shared pointer destructor invokes it. - bool found_deleter = false; - if (gd) { - found_deleter = gd->extract_deleter(extracted_deleter); - } + const bool found_deleter = gd && gd->extract_deleter(extracted_deleter); release_ownership(); + // If we have a deleter, pass that onto the unique pointer. if (found_deleter) { return std::unique_ptr(raw_ptr, std::move(extracted_deleter)); } + // No special destructor. return std::unique_ptr(raw_ptr); }