From 7cc1e1f7f96700a2cf5a7e7e86a9b32ec783d505 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Sat, 4 Nov 2023 12:30:49 -0400 Subject: [PATCH 01/12] Roundtrip through unique pointer with custom deleter. Currently failing. --- tests/test_class_sh_basic.cpp | 20 ++++++++++++++++++++ tests/test_class_sh_basic.py | 4 ++++ 2 files changed, 24 insertions(+) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 7582cfa044..69003c2b3b 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -64,6 +64,23 @@ std::unique_ptr rtrn_udcp() { return std::unique_ptr obj) { return "pass_udmp:" + obj->mtxt; } std::string pass_udcp(std::unique_ptr obj) { return "pass_udcp:" + obj->mtxt; } + +struct custom_deleter +{ + std::string delete_txt; + void operator()(atyp* p) const + { + std::default_delete()(p); + } +}; + +std::unique_ptr rtrn_udmp_del() { return std::unique_ptr(new atyp{"rtrn_udmp_del"}, custom_deleter{"udmp_deleter"}); } + +std::string pass_udmp_del(std::unique_ptr obj) { + const auto d = obj.get_deleter(); + return "pass_udmp_del:" + obj->mtxt + "," + d.delete_txt; +} + // clang-format on // Helpers for testing. @@ -130,6 +147,9 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("pass_udmp", pass_udmp); m.def("pass_udcp", pass_udcp); + m.def("rtrn_udmp_del", rtrn_udmp_del); + m.def("pass_udmp_del", pass_udmp_del); + py::classh(m, "uconsumer") .def(py::init<>()) .def("valid", &uconsumer::valid) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 268e793985..696e6e5129 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -64,6 +64,10 @@ def test_load_with_mtxt(pass_f, mtxt, expected): def test_load_with_rtrn_f(pass_f, rtrn_f, expected): assert pass_f(rtrn_f()) == expected +def test_deleter_roundtrip(): + t = m.rtrn_udmp_del() + r = m.pass_udmp_del(t) + assert r == "pass_udmp_del:rtrn_udmp_del,udmp_deleter" @pytest.mark.parametrize( ("pass_f", "rtrn_f", "expected"), From 5b00921098ab97001c602fe07c1a1e781efb17ad Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Sat, 4 Nov 2023 14:46:58 -0400 Subject: [PATCH 02/12] Ensure the custom deleter is copied back to the unique pointer. Feels like there's still a gap around the raw pointer flavour, but this at least makes the unit test of the previous commit succeed. --- .../detail/smart_holder_type_casters.h | 58 ++++++++++++++++++- tests/test_class_sh_basic.cpp | 15 ++--- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index bd9483a025..979c2fb1e5 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -464,6 +464,29 @@ struct shared_ptr_trampoline_self_life_support { } }; +template +struct delete_assigner { + static void assign(D &, bool, std::function &, void (*)(void *) &) { + // Situation where the deleter cannot be assigned from either del_fun or del_ptr. + // This covers the default deleters and the like. + } +}; + +template +struct delete_assigner { + static void assign(D &deleter, + bool use_del_fun, + std::function &del_fun, + void (*del_ptr)(void *) &) { + // Situation where D is assignable from del_fun. + if (use_del_fun) { + deleter = std::move(del_fun); + } else { + deleter = del_ptr; + } + } +}; + template struct smart_holder_type_caster_load { using holder_type = pybindit::memory::smart_holder; @@ -616,13 +639,46 @@ struct smart_holder_type_caster_load { "instance cannot safely be transferred to C++."); } + // Need to extract the deleter from the holder such that it can be passed back to the + // unique pointer. + + // Temporary variable to store the extracted deleter in. + D extracted_deleter; + + // In smart_holder_poc, the deleter is always stored in a guarded delete. + // The guarded delete's std::function actually points at the custom_deleter + // type, so we can verify it is of the custom deleter type and finally extract its deleter. + auto *gd = std::get_deleter(holder().vptr); + if (gd) { + if (gd->use_del_fun) { + using custom_deleter_D = pybindit::memory::custom_deleter; + const auto &custom_deleter_ptr = gd->del_fun.template target(); + if (!custom_deleter_ptr) { + throw cast_error("Deletion function is not stored in custom deleter, cannot \ + extract the deleter correctly."); + } + // Now that we have confirmed the type of the deleter matches the desired return + // value we can extract the function. + extracted_deleter = std::move(custom_deleter_ptr->deleter); + } else { + // The del_ptr attribute of the guarded deleter does not provide any type + // information that can be used to confirm it is convertible to D. SFINEA here is + // necessary to ensure that if D is not constructible from a void(void*) pointer, + // it does not cause compilation failures. If this hits an non-convertible type at + // compile time it throws. + constexpr bool assignable = std::is_constructibledel_fun)>::value; + delete_assigner::assign( + extracted_deleter, gd->use_del_fun, gd->del_fun, gd->del_ptr); + } + } + // Critical transfer-of-ownership section. This must stay together. if (self_life_support != nullptr) { holder().disown(); } else { holder().release_ownership(); } - auto result = std::unique_ptr(raw_type_ptr); + auto result = std::unique_ptr(raw_type_ptr, std::move(extracted_deleter)); if (self_life_support != nullptr) { self_life_support->activate_life_support(load_impl.loaded_v_h); } else { diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 69003c2b3b..a01a9b3e5f 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -67,19 +67,16 @@ std::string pass_udcp(std::unique_ptr obj) { return "pass_udcp struct custom_deleter { - std::string delete_txt; - void operator()(atyp* p) const - { - std::default_delete()(p); - } + std::string delete_txt; + void operator()(atyp* p) const + { + std::default_delete()(p); + } }; std::unique_ptr rtrn_udmp_del() { return std::unique_ptr(new atyp{"rtrn_udmp_del"}, custom_deleter{"udmp_deleter"}); } -std::string pass_udmp_del(std::unique_ptr obj) { - const auto d = obj.get_deleter(); - return "pass_udmp_del:" + obj->mtxt + "," + d.delete_txt; -} +std::string pass_udmp_del(std::unique_ptr obj) { return "pass_udmp_del:" + obj->mtxt + "," + obj.get_deleter().delete_txt; } // clang-format on From ad7eaa4310fd8ea8fe67bd8e0f876a55a114aa45 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Sun, 5 Nov 2023 09:25:24 -0500 Subject: [PATCH 03/12] Add deleter roundtrip for const atyp. Currently failing, custom deleter is lost. --- tests/test_class_sh_basic.cpp | 9 +++++++++ tests/test_class_sh_basic.py | 13 +++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index a01a9b3e5f..9162dd3f47 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -72,11 +72,17 @@ struct custom_deleter { std::default_delete()(p); } + void operator()(const atyp* p) const + { + std::default_delete()(p); + } }; std::unique_ptr rtrn_udmp_del() { return std::unique_ptr(new atyp{"rtrn_udmp_del"}, custom_deleter{"udmp_deleter"}); } +std::unique_ptr rtrn_udcp_del() { return std::unique_ptr(new atyp{"rtrn_udcp_del"}, custom_deleter{"udcp_deleter"}); } std::string pass_udmp_del(std::unique_ptr obj) { return "pass_udmp_del:" + obj->mtxt + "," + obj.get_deleter().delete_txt; } +std::string pass_udcp_del(std::unique_ptr obj) { return "pass_udcp_del:" + obj->mtxt + "," + obj.get_deleter().delete_txt; } // clang-format on @@ -145,7 +151,10 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("pass_udcp", pass_udcp); m.def("rtrn_udmp_del", rtrn_udmp_del); + m.def("rtrn_udcp_del", rtrn_udcp_del); + m.def("pass_udmp_del", pass_udmp_del); + m.def("pass_udcp_del", pass_udcp_del); py::classh(m, "uconsumer") .def(py::init<>()) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 696e6e5129..e3fe1df129 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -64,10 +64,15 @@ def test_load_with_mtxt(pass_f, mtxt, expected): def test_load_with_rtrn_f(pass_f, rtrn_f, expected): assert pass_f(rtrn_f()) == expected -def test_deleter_roundtrip(): - t = m.rtrn_udmp_del() - r = m.pass_udmp_del(t) - assert r == "pass_udmp_del:rtrn_udmp_del,udmp_deleter" +@pytest.mark.parametrize( + ("pass_f", "rtrn_f", "expected"), + [ + (m.pass_udmp_del, m.rtrn_udmp_del, "pass_udmp_del:rtrn_udmp_del,udmp_deleter"), + (m.pass_udcp_del, m.rtrn_udcp_del, "pass_udcp_del:rtrn_udcp_del,udcp_deleter"), + ], +) +def test_deleter_roundtrip(pass_f, rtrn_f, expected): + assert pass_f(rtrn_f()) == expected @pytest.mark.parametrize( ("pass_f", "rtrn_f", "expected"), From a49a7ce819f88e372af800ec6971eddbe254286f Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Sun, 5 Nov 2023 10:21:48 -0500 Subject: [PATCH 04/12] Fix storing deleter for const unique ptr. Unit test from the previous commit passes. --- include/pybind11/detail/smart_holder_type_casters.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 979c2fb1e5..19154e049b 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -1112,7 +1112,8 @@ struct smart_holder_type_caster> static handle cast(std::unique_ptr &&src, return_value_policy policy, handle parent) { return smart_holder_type_caster>::cast( - std::unique_ptr(const_cast(src.release())), // Const2Mutbl + std::unique_ptr(const_cast(src.release()), + std::move(src.get_deleter())), // Const2Mutbl policy, parent); } From eb05f97c2144b43099c0707fe25e9c1a17734d7f Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Sun, 5 Nov 2023 11:07:07 -0500 Subject: [PATCH 05/12] Remove SFINEA deleter assignment. At the construction of the smart holder, it is either a del_fun, or a default constructed deleter, so this complexity is unnecessary. --- .../detail/smart_holder_type_casters.h | 46 ++++--------------- 1 file changed, 8 insertions(+), 38 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 19154e049b..2aafe84aee 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -464,29 +464,6 @@ struct shared_ptr_trampoline_self_life_support { } }; -template -struct delete_assigner { - static void assign(D &, bool, std::function &, void (*)(void *) &) { - // Situation where the deleter cannot be assigned from either del_fun or del_ptr. - // This covers the default deleters and the like. - } -}; - -template -struct delete_assigner { - static void assign(D &deleter, - bool use_del_fun, - std::function &del_fun, - void (*del_ptr)(void *) &) { - // Situation where D is assignable from del_fun. - if (use_del_fun) { - deleter = std::move(del_fun); - } else { - deleter = del_ptr; - } - } -}; - template struct smart_holder_type_caster_load { using holder_type = pybindit::memory::smart_holder; @@ -639,18 +616,16 @@ struct smart_holder_type_caster_load { "instance cannot safely be transferred to C++."); } - // Need to extract the deleter from the holder such that it can be passed back to the - // unique pointer. - - // Temporary variable to store the extracted deleter in. + // Default constructed temporary variable to store the extracted deleter in. D extracted_deleter; - // In smart_holder_poc, the deleter is always stored in a guarded delete. - // The guarded delete's std::function actually points at the custom_deleter - // type, so we can verify it is of the custom deleter type and finally extract its deleter. auto *gd = std::get_deleter(holder().vptr); if (gd) { if (gd->use_del_fun) { + // In smart_holder_poc, a custom deleter is always stored in a guarded delete. + // The guarded delete's std::function actually points at the + // custom_deleter type, so we can verify it is of the custom deleter type and + // finally extract its deleter. using custom_deleter_D = pybindit::memory::custom_deleter; const auto &custom_deleter_ptr = gd->del_fun.template target(); if (!custom_deleter_ptr) { @@ -661,14 +636,9 @@ struct smart_holder_type_caster_load { // value we can extract the function. extracted_deleter = std::move(custom_deleter_ptr->deleter); } else { - // The del_ptr attribute of the guarded deleter does not provide any type - // information that can be used to confirm it is convertible to D. SFINEA here is - // necessary to ensure that if D is not constructible from a void(void*) pointer, - // it does not cause compilation failures. If this hits an non-convertible type at - // compile time it throws. - constexpr bool assignable = std::is_constructibledel_fun)>::value; - delete_assigner::assign( - extracted_deleter, gd->use_del_fun, gd->del_fun, gd->del_ptr); + // Not sure if anything needs to be done here. In general, if the del function is + // used a default destructor is used which should be accomodated by the type of the + // deleter used in the returned unique ptr. } } From 23d5eac27bcb3d1579b9c2277288d48cd4d7dd9d Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Sun, 5 Nov 2023 11:09:11 -0500 Subject: [PATCH 06/12] Clang format. --- include/pybind11/detail/smart_holder_type_casters.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 2aafe84aee..ee7b79ff31 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -1083,7 +1083,7 @@ struct smart_holder_type_caster> cast(std::unique_ptr &&src, return_value_policy policy, handle parent) { return smart_holder_type_caster>::cast( std::unique_ptr(const_cast(src.release()), - std::move(src.get_deleter())), // Const2Mutbl + std::move(src.get_deleter())), // Const2Mutbl policy, parent); } From 88008001297010f10e1529ccebe814e645268af6 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Sun, 5 Nov 2023 12:19:00 -0500 Subject: [PATCH 07/12] Fixes for ci. Clang 3.6 requires the extra constructors in the custom_deleter. --- include/pybind11/detail/smart_holder_type_casters.h | 4 ++-- tests/test_class_sh_basic.cpp | 4 +++- tests/test_class_sh_basic.py | 2 ++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index ee7b79ff31..4405d20378 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -637,8 +637,8 @@ struct smart_holder_type_caster_load { extracted_deleter = std::move(custom_deleter_ptr->deleter); } else { // Not sure if anything needs to be done here. In general, if the del function is - // used a default destructor is used which should be accomodated by the type of the - // deleter used in the returned unique ptr. + // used a default destructor is used which should be accommodated by the type of + // the deleter used in the returned unique ptr. } } diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 9162dd3f47..0440f3e12d 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -68,6 +68,8 @@ std::string pass_udcp(std::unique_ptr obj) { return "pass_udcp struct custom_deleter { std::string delete_txt; + custom_deleter() = default; + explicit custom_deleter(const std::string &delete_txt_) : delete_txt(delete_txt_) {} void operator()(atyp* p) const { std::default_delete()(p); @@ -80,7 +82,7 @@ struct custom_deleter std::unique_ptr rtrn_udmp_del() { return std::unique_ptr(new atyp{"rtrn_udmp_del"}, custom_deleter{"udmp_deleter"}); } std::unique_ptr rtrn_udcp_del() { return std::unique_ptr(new atyp{"rtrn_udcp_del"}, custom_deleter{"udcp_deleter"}); } - + std::string pass_udmp_del(std::unique_ptr obj) { return "pass_udmp_del:" + obj->mtxt + "," + obj.get_deleter().delete_txt; } std::string pass_udcp_del(std::unique_ptr obj) { return "pass_udcp_del:" + obj->mtxt + "," + obj.get_deleter().delete_txt; } diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index e3fe1df129..72669591f9 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -64,6 +64,7 @@ def test_load_with_mtxt(pass_f, mtxt, expected): def test_load_with_rtrn_f(pass_f, rtrn_f, expected): assert pass_f(rtrn_f()) == expected + @pytest.mark.parametrize( ("pass_f", "rtrn_f", "expected"), [ @@ -74,6 +75,7 @@ def test_load_with_rtrn_f(pass_f, rtrn_f, expected): def test_deleter_roundtrip(pass_f, rtrn_f, expected): assert pass_f(rtrn_f()) == expected + @pytest.mark.parametrize( ("pass_f", "rtrn_f", "expected"), [ From f2f87f0cc143be54060ae670c3ef2e073e621191 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Mon, 6 Nov 2023 18:40:41 -0500 Subject: [PATCH 08/12] fix(smart_holder): Loosen requirement on deleter to be default constructible. And some other PR feedback. --- .../detail/smart_holder_type_casters.h | 57 +++++++++++-------- tests/test_class_sh_basic.cpp | 2 +- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 4405d20378..42f97b1c21 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -464,6 +464,24 @@ struct shared_ptr_trampoline_self_life_support { } }; +template ::value, int>::type = 0> +inline std::unique_ptr unique_with_deleter(T *raw_ptr, std::unique_ptr &&deleter) { + if (deleter != nullptr) { + return std::unique_ptr(raw_ptr, std::move(*deleter)); + } + return std::unique_ptr(raw_ptr); +} + +template ::value, int>::type = 0> +inline std::unique_ptr unique_with_deleter(T *raw_ptr, std::unique_ptr &&deleter) { + assert(deleter != nullptr); + return std::unique_ptr(raw_ptr, std::move(*deleter)); +} + template struct smart_holder_type_caster_load { using holder_type = pybindit::memory::smart_holder; @@ -589,7 +607,7 @@ struct smart_holder_type_caster_load { " std::unique_ptr."); } if (!have_holder()) { - return nullptr; + return unique_with_deleter(nullptr, std::unique_ptr()); } throw_if_uninitialized_or_disowned_holder(); throw_if_instance_is_currently_owned_by_shared_ptr(); @@ -616,30 +634,21 @@ struct smart_holder_type_caster_load { "instance cannot safely be transferred to C++."); } - // Default constructed temporary variable to store the extracted deleter in. - D extracted_deleter; + // Temporary variable to store the extracted deleter in. + std::unique_ptr extracted_deleter; auto *gd = std::get_deleter(holder().vptr); - if (gd) { - if (gd->use_del_fun) { - // In smart_holder_poc, a custom deleter is always stored in a guarded delete. - // The guarded delete's std::function actually points at the - // custom_deleter type, so we can verify it is of the custom deleter type and - // finally extract its deleter. - using custom_deleter_D = pybindit::memory::custom_deleter; - const auto &custom_deleter_ptr = gd->del_fun.template target(); - if (!custom_deleter_ptr) { - throw cast_error("Deletion function is not stored in custom deleter, cannot \ - extract the deleter correctly."); - } - // Now that we have confirmed the type of the deleter matches the desired return - // value we can extract the function. - extracted_deleter = std::move(custom_deleter_ptr->deleter); - } else { - // Not sure if anything needs to be done here. In general, if the del function is - // used a default destructor is used which should be accommodated by the type of - // the deleter used in the returned unique ptr. - } + if (gd && gd->use_del_fun) { // Note the ensure_compatible_rtti_uqp_del() call above. + // In smart_holder_poc, a custom deleter is always stored in a guarded delete. + // The guarded delete's std::function actually points at the + // custom_deleter type, so we can verify it is of the custom deleter type and + // finally extract its deleter. + using custom_deleter_D = pybindit::memory::custom_deleter; + const auto &custom_deleter_ptr = gd->del_fun.template target(); + assert(custom_deleter_ptr != nullptr); + // Now that we have confirmed the type of the deleter matches the desired return + // value we can extract the function. + extracted_deleter = std::unique_ptr(new D(std::move(custom_deleter_ptr->deleter))); } // Critical transfer-of-ownership section. This must stay together. @@ -648,7 +657,7 @@ struct smart_holder_type_caster_load { } else { holder().release_ownership(); } - auto result = std::unique_ptr(raw_type_ptr, std::move(extracted_deleter)); + auto result = unique_with_deleter(raw_type_ptr, std::move(extracted_deleter)); if (self_life_support != nullptr) { self_life_support->activate_life_support(load_impl.loaded_v_h); } else { diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 0440f3e12d..99a1460665 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -68,7 +68,7 @@ std::string pass_udcp(std::unique_ptr obj) { return "pass_udcp struct custom_deleter { std::string delete_txt; - custom_deleter() = default; + custom_deleter() = delete; explicit custom_deleter(const std::string &delete_txt_) : delete_txt(delete_txt_) {} void operator()(atyp* p) const { From 13105d574094ffa8ed50499a5eb37f9902128694 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Tue, 7 Nov 2023 07:33:35 -0500 Subject: [PATCH 09/12] fix(smart_holder): Custom deleter in unit tests traces constructions. --- tests/test_class_sh_basic.cpp | 47 ++++++++++++++++++++++------------- tests/test_class_sh_basic.py | 4 +-- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 99a1460665..55d934c347 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -28,6 +28,34 @@ struct uconsumer { // unique_ptr consumer const std::unique_ptr &rtrn_cref() const { return held; } }; +struct custom_deleter { + std::string trace_txt; + + custom_deleter() = delete; + explicit custom_deleter(const std::string &trace_txt_) : trace_txt(trace_txt_) {} + + custom_deleter(const custom_deleter &other) { trace_txt = other.trace_txt + "_CpCtor"; } + + custom_deleter &operator=(const custom_deleter &rhs) { + trace_txt = rhs.trace_txt + "_CpLhs"; + return *this; + } + + custom_deleter(custom_deleter &&other) { + trace_txt = other.trace_txt + "_MvCtorTo"; + other.trace_txt += "_MvCtorFrom"; + } + + custom_deleter &operator=(custom_deleter &&rhs) { + trace_txt = rhs.trace_txt + "_MvLhs"; + rhs.trace_txt += "_MvRhs"; + return *this; + } + + void operator()(atyp *p) const { std::default_delete()(p); } + void operator()(const atyp *p) const { std::default_delete()(p); } +}; + // clang-format off atyp rtrn_valu() { atyp obj{"rtrn_valu"}; return obj; } @@ -65,26 +93,11 @@ std::string pass_udmp(std::unique_ptr obj) { return "pass_udmp std::string pass_udcp(std::unique_ptr obj) { return "pass_udcp:" + obj->mtxt; } -struct custom_deleter -{ - std::string delete_txt; - custom_deleter() = delete; - explicit custom_deleter(const std::string &delete_txt_) : delete_txt(delete_txt_) {} - void operator()(atyp* p) const - { - std::default_delete()(p); - } - void operator()(const atyp* p) const - { - std::default_delete()(p); - } -}; - std::unique_ptr rtrn_udmp_del() { return std::unique_ptr(new atyp{"rtrn_udmp_del"}, custom_deleter{"udmp_deleter"}); } std::unique_ptr rtrn_udcp_del() { return std::unique_ptr(new atyp{"rtrn_udcp_del"}, custom_deleter{"udcp_deleter"}); } -std::string pass_udmp_del(std::unique_ptr obj) { return "pass_udmp_del:" + obj->mtxt + "," + obj.get_deleter().delete_txt; } -std::string pass_udcp_del(std::unique_ptr obj) { return "pass_udcp_del:" + obj->mtxt + "," + obj.get_deleter().delete_txt; } +std::string pass_udmp_del(std::unique_ptr obj) { return "pass_udmp_del:" + obj->mtxt + "," + obj.get_deleter().trace_txt; } +std::string pass_udcp_del(std::unique_ptr obj) { return "pass_udcp_del:" + obj->mtxt + "," + obj.get_deleter().trace_txt; } // clang-format on diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 72669591f9..5f44150891 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -68,8 +68,8 @@ def test_load_with_rtrn_f(pass_f, rtrn_f, expected): @pytest.mark.parametrize( ("pass_f", "rtrn_f", "expected"), [ - (m.pass_udmp_del, m.rtrn_udmp_del, "pass_udmp_del:rtrn_udmp_del,udmp_deleter"), - (m.pass_udcp_del, m.rtrn_udcp_del, "pass_udcp_del:rtrn_udcp_del,udcp_deleter"), + (m.pass_udmp_del, m.rtrn_udmp_del, "pass_udmp_del:rtrn_udmp_del,udmp_deleter_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo"), + (m.pass_udcp_del, m.rtrn_udcp_del, "pass_udcp_del:rtrn_udcp_del,udcp_deleter_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo"), ], ) def test_deleter_roundtrip(pass_f, rtrn_f, expected): From 4cdd8bbb65bf1e964106e78428095fad80682509 Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Tue, 7 Nov 2023 07:39:10 -0500 Subject: [PATCH 10/12] fix(smart_holder): Use pybind11_fail instead of assert. --- include/pybind11/detail/smart_holder_type_casters.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 42f97b1c21..bfe54c847a 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -468,17 +468,20 @@ template ::value, int>::type = 0> inline std::unique_ptr unique_with_deleter(T *raw_ptr, std::unique_ptr &&deleter) { - if (deleter != nullptr) { - return std::unique_ptr(raw_ptr, std::move(*deleter)); + if (deleter == nullptr) { + return std::unique_ptr(raw_ptr); } - return std::unique_ptr(raw_ptr); + return std::unique_ptr(raw_ptr, std::move(*deleter)); } template ::value, int>::type = 0> inline std::unique_ptr unique_with_deleter(T *raw_ptr, std::unique_ptr &&deleter) { - assert(deleter != nullptr); + if (deleter == nullptr) { + pybind11_fail("smart_holder_type_casters: deleter is not default constructible and no" + " instance available to return."); + } return std::unique_ptr(raw_ptr, std::move(*deleter)); } From 5c062eb6cbe0c9e5db602bcc20821534d6d2f76f Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Tue, 7 Nov 2023 08:01:02 -0500 Subject: [PATCH 11/12] fix(smart_holder): Add unit tests for the default constructible deleter. --- tests/test_class_sh_basic.cpp | 24 ++++++++++++++++++++++-- tests/test_class_sh_basic.py | 8 ++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 55d934c347..d48aca9c24 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -28,10 +28,11 @@ struct uconsumer { // unique_ptr consumer const std::unique_ptr &rtrn_cref() const { return held; } }; +/// Custom deleter that is default constructible. struct custom_deleter { std::string trace_txt; - custom_deleter() = delete; + custom_deleter() = default; explicit custom_deleter(const std::string &trace_txt_) : trace_txt(trace_txt_) {} custom_deleter(const custom_deleter &other) { trace_txt = other.trace_txt + "_CpCtor"; } @@ -55,6 +56,14 @@ struct custom_deleter { void operator()(atyp *p) const { std::default_delete()(p); } void operator()(const atyp *p) const { std::default_delete()(p); } }; +static_assert(std::is_default_constructible::value, ""); + +/// Custom deleter that is not default constructible. +struct custom_deleter_nd : custom_deleter { + custom_deleter_nd() = delete; + explicit custom_deleter_nd(const std::string &trace_txt_) : custom_deleter(trace_txt_) {} +}; +static_assert(!std::is_default_constructible::value, ""); // clang-format off @@ -92,13 +101,18 @@ std::unique_ptr rtrn_udcp() { return std::unique_ptr obj) { return "pass_udmp:" + obj->mtxt; } std::string pass_udcp(std::unique_ptr obj) { return "pass_udcp:" + obj->mtxt; } - std::unique_ptr rtrn_udmp_del() { return std::unique_ptr(new atyp{"rtrn_udmp_del"}, custom_deleter{"udmp_deleter"}); } std::unique_ptr rtrn_udcp_del() { return std::unique_ptr(new atyp{"rtrn_udcp_del"}, custom_deleter{"udcp_deleter"}); } std::string pass_udmp_del(std::unique_ptr obj) { return "pass_udmp_del:" + obj->mtxt + "," + obj.get_deleter().trace_txt; } std::string pass_udcp_del(std::unique_ptr obj) { return "pass_udcp_del:" + obj->mtxt + "," + obj.get_deleter().trace_txt; } +std::unique_ptr rtrn_udmp_del_nd() { return std::unique_ptr(new atyp{"rtrn_udmp_del_nd"}, custom_deleter_nd{"udmp_deleter_nd"}); } +std::unique_ptr rtrn_udcp_del_nd() { return std::unique_ptr(new atyp{"rtrn_udcp_del_nd"}, custom_deleter_nd{"udcp_deleter_nd"}); } + +std::string pass_udmp_del_nd(std::unique_ptr obj) { return "pass_udmp_del_nd:" + obj->mtxt + "," + obj.get_deleter().trace_txt; } +std::string pass_udcp_del_nd(std::unique_ptr obj) { return "pass_udcp_del_nd:" + obj->mtxt + "," + obj.get_deleter().trace_txt; } + // clang-format on // Helpers for testing. @@ -171,6 +185,12 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("pass_udmp_del", pass_udmp_del); m.def("pass_udcp_del", pass_udcp_del); + m.def("rtrn_udmp_del_nd", rtrn_udmp_del_nd); + m.def("rtrn_udcp_del_nd", rtrn_udcp_del_nd); + + m.def("pass_udmp_del_nd", pass_udmp_del_nd); + m.def("pass_udcp_del_nd", pass_udcp_del_nd); + py::classh(m, "uconsumer") .def(py::init<>()) .def("valid", &uconsumer::valid) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 5f44150891..b3e9709dd2 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -68,8 +68,12 @@ def test_load_with_rtrn_f(pass_f, rtrn_f, expected): @pytest.mark.parametrize( ("pass_f", "rtrn_f", "expected"), [ - (m.pass_udmp_del, m.rtrn_udmp_del, "pass_udmp_del:rtrn_udmp_del,udmp_deleter_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo"), - (m.pass_udcp_del, m.rtrn_udcp_del, "pass_udcp_del:rtrn_udcp_del,udcp_deleter_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo_MvCtorTo"), + (m.pass_udmp_del, m.rtrn_udmp_del, "pass_udmp_del:rtrn_udmp_del,udmp_deleter_" + "_".join(["MvCtorTo"] * 6)), + (m.pass_udcp_del, m.rtrn_udcp_del, "pass_udcp_del:rtrn_udcp_del,udcp_deleter_" + "_".join(["MvCtorTo"] * 8)), + (m.pass_udmp_del_nd, m.rtrn_udmp_del_nd, + "pass_udmp_del_nd:rtrn_udmp_del_nd,udmp_deleter_nd_" + "_".join(["MvCtorTo"] * 6)), + (m.pass_udcp_del_nd, m.rtrn_udcp_del_nd, + "pass_udcp_del_nd:rtrn_udcp_del_nd,udcp_deleter_nd_" + "_".join(["MvCtorTo"] * 8)), ], ) def test_deleter_roundtrip(pass_f, rtrn_f, expected): From 8a75f12a60f9c883b1eb1635bf5e798a5ce54b6b Mon Sep 17 00:00:00 2001 From: Ivor Wanders Date: Tue, 7 Nov 2023 18:37:53 -0500 Subject: [PATCH 12/12] fix(smart_holder): Use regex matching for deleter constructors in unit tests. --- tests/test_class_sh_basic.cpp | 4 ++-- tests/test_class_sh_basic.py | 32 +++++++++++++++++++++++--------- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index d48aca9c24..a294f7bbb2 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -42,12 +42,12 @@ struct custom_deleter { return *this; } - custom_deleter(custom_deleter &&other) { + custom_deleter(custom_deleter &&other) noexcept { trace_txt = other.trace_txt + "_MvCtorTo"; other.trace_txt += "_MvCtorFrom"; } - custom_deleter &operator=(custom_deleter &&rhs) { + custom_deleter &operator=(custom_deleter &&rhs) noexcept { trace_txt = rhs.trace_txt + "_MvLhs"; rhs.trace_txt += "_MvRhs"; return *this; diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index b3e9709dd2..7be0747c2a 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -66,18 +66,32 @@ def test_load_with_rtrn_f(pass_f, rtrn_f, expected): @pytest.mark.parametrize( - ("pass_f", "rtrn_f", "expected"), + ("pass_f", "rtrn_f", "regex_expected"), [ - (m.pass_udmp_del, m.rtrn_udmp_del, "pass_udmp_del:rtrn_udmp_del,udmp_deleter_" + "_".join(["MvCtorTo"] * 6)), - (m.pass_udcp_del, m.rtrn_udcp_del, "pass_udcp_del:rtrn_udcp_del,udcp_deleter_" + "_".join(["MvCtorTo"] * 8)), - (m.pass_udmp_del_nd, m.rtrn_udmp_del_nd, - "pass_udmp_del_nd:rtrn_udmp_del_nd,udmp_deleter_nd_" + "_".join(["MvCtorTo"] * 6)), - (m.pass_udcp_del_nd, m.rtrn_udcp_del_nd, - "pass_udcp_del_nd:rtrn_udcp_del_nd,udcp_deleter_nd_" + "_".join(["MvCtorTo"] * 8)), + ( + m.pass_udmp_del, + m.rtrn_udmp_del, + "pass_udmp_del:rtrn_udmp_del,udmp_deleter(_MvCtorTo)*_MvCtorTo", + ), + ( + m.pass_udcp_del, + m.rtrn_udcp_del, + "pass_udcp_del:rtrn_udcp_del,udcp_deleter(_MvCtorTo)*_MvCtorTo", + ), + ( + m.pass_udmp_del_nd, + m.rtrn_udmp_del_nd, + "pass_udmp_del_nd:rtrn_udmp_del_nd,udmp_deleter_nd(_MvCtorTo)*_MvCtorTo", + ), + ( + m.pass_udcp_del_nd, + m.rtrn_udcp_del_nd, + "pass_udcp_del_nd:rtrn_udcp_del_nd,udcp_deleter_nd(_MvCtorTo)*_MvCtorTo", + ), ], ) -def test_deleter_roundtrip(pass_f, rtrn_f, expected): - assert pass_f(rtrn_f()) == expected +def test_deleter_roundtrip(pass_f, rtrn_f, regex_expected): + assert re.match(regex_expected, pass_f(rtrn_f())) @pytest.mark.parametrize(