fix(smart_holder): Loosen requirement on deleter to be default constructible.

iwanders · iwanders · commit f2f87f0cc143 · 2023-11-06T19:19:24.000-05:00
And some other PR feedback.
diff --git 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 <typename T,
+          typename D,
+          typename std::enable_if<std::is_default_constructible<D>::value, int>::type = 0>
+inline std::unique_ptr<T, D> unique_with_deleter(T *raw_ptr, std::unique_ptr<D> &&deleter) {
+    if (deleter != nullptr) {
+        return std::unique_ptr<T, D>(raw_ptr, std::move(*deleter));
+    }
+    return std::unique_ptr<T, D>(raw_ptr);
+}
+
+template <typename T,
+          typename D,
+          typename std::enable_if<!std::is_default_constructible<D>::value, int>::type = 0>
+inline std::unique_ptr<T, D> unique_with_deleter(T *raw_ptr, std::unique_ptr<D> &&deleter) {
+    assert(deleter != nullptr);
+    return std::unique_ptr<T, D>(raw_ptr, std::move(*deleter));
+}
+
 template <typename T>
 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<T, D>(nullptr, std::unique_ptr<D>());
         }
         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<D> extracted_deleter;
 
         auto *gd = std::get_deleter<pybindit::memory::guarded_delete>(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<void(void*)> 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<T, D>;
-                const auto &custom_deleter_ptr = gd->del_fun.template target<custom_deleter_D>();
-                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<T, D>() call above.
+            // In smart_holder_poc, a custom  deleter is always stored in a guarded delete.
+            // The guarded delete's std::function<void(void*)> 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<T, D>;
+            const auto &custom_deleter_ptr = gd->del_fun.template target<custom_deleter_D>();
+            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<D>(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<T, D>(raw_type_ptr, std::move(extracted_deleter));
+        auto result = unique_with_deleter<T, D>(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
@@ -68,7 +68,7 @@ std::string pass_udcp(std::unique_ptr<atyp const, sddc> 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
     {

Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,7 @@ std::string pass_udcp(std::unique_ptr<atyp const, sddc> obj) { return "pass_udcp`
`68`	`68`	`struct custom_deleter`
`69`	`69`	`{`
`70`	`70`	`std::string delete_txt;`
`71`		`- custom_deleter() = default;`
	`71`	`+ custom_deleter() = delete;`
`72`	`72`	`explicit custom_deleter(const std::string &delete_txt_) : delete_txt(delete_txt_) {}`
`73`	`73`	`void operator()(atyp* p) const`
`74`	`74`	`{`