From 62bb88ed57c0ae9fb1109ec5459b7feb6980d287 Mon Sep 17 00:00:00 2001
From: Ivor Wanders <ivor@iwanders.net>
Date: Mon, 6 Nov 2023 18:40:41 -0500
Subject: [PATCH] fix(smart_holder): Loosen requirement on deleter to be
 default constructible.

And some other PR feedback.
---
 .../detail/smart_holder_type_casters.h        | 58 +++++++++++--------
 tests/test_class_sh_basic.cpp                 |  4 +-
 2 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h
index 4405d20378a..6baad3ea6ac 100644
--- a/include/pybind11/detail/smart_holder_type_casters.h
+++ b/include/pybind11/detail/smart_holder_type_casters.h
@@ -464,6 +464,25 @@ 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));
+    } else {
+        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 +608,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 +635,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::make_unique<D>(std::move(custom_deleter_ptr->deleter));
         }
 
         // Critical transfer-of-ownership section. This must stay together.
@@ -648,7 +658,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
index 0440f3e12db..65c75c583ce 100644
--- a/tests/test_class_sh_basic.cpp
+++ b/tests/test_class_sh_basic.cpp
@@ -68,8 +68,10 @@ 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;
+#if (defined(__clang_major__) && __clang_major__ == 3 && __clang_minor__ == 6)
     explicit custom_deleter(const std::string &delete_txt_) : delete_txt(delete_txt_) {}
+#endif
     void operator()(atyp* p) const
     {
         std::default_delete<atyp>()(p);