Ensure the custom deleter is copied back to the unique pointer.

iwanders · iwanders · commit 5b00921098ab · 2023-11-04T14:46:58.000-04:00
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.
diff --git 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 <typename D, bool>
+struct delete_assigner {
+    static void assign(D &, bool, std::function<void(void *)> &, 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 <typename D>
+struct delete_assigner<D, true> {
+    static void assign(D &deleter,
+                       bool use_del_fun,
+                       std::function<void(void *)> &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 <typename T>
 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<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.
+        auto *gd = std::get_deleter<pybindit::memory::guarded_delete>(holder().vptr);
+        if (gd) {
+            if (gd->use_del_fun) {
+                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 {
+                // 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_constructible<D, decltype(gd->del_fun)>::value;
+                delete_assigner<D, assignable>::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<T, D>(raw_type_ptr);
+        auto result = std::unique_ptr<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
@@ -67,19 +67,16 @@ std::string pass_udcp(std::unique_ptr<atyp const, sddc> obj) { return "pass_udcp
 
 struct custom_deleter
 {
-  std::string delete_txt;
-  void operator()(atyp* p) const
-  {
-    std::default_delete<atyp>()(p);
-  }
+    std::string delete_txt;
+    void operator()(atyp* p) const
+    {
+        std::default_delete<atyp>()(p);
+    }
 };
 
 std::unique_ptr<atyp,       custom_deleter> rtrn_udmp_del() { return std::unique_ptr<atyp,       custom_deleter>(new atyp{"rtrn_udmp_del"}, custom_deleter{"udmp_deleter"}); }
  
-std::string pass_udmp_del(std::unique_ptr<atyp,       custom_deleter> obj) {
-  const auto d = obj.get_deleter();
-  return "pass_udmp_del:" + obj->mtxt + "," + d.delete_txt;
-}
+std::string pass_udmp_del(std::unique_ptr<atyp,       custom_deleter> obj) { return "pass_udmp_del:" + obj->mtxt + "," + obj.get_deleter().delete_txt; }
 
 // clang-format on
 
