py::smart_holder std::shared_ptr deleter simplification & optimization. (#3041)

Author: rwgk

include/pybind11/detail/smart_holder_poc.h

@@ -56,36 +56,43 @@ High-level aspects:
 namespace pybindit {
 namespace memory {
 
-template <typename T>
-struct guarded_builtin_delete {
-    std::shared_ptr<bool> flag_ptr;
-    explicit guarded_builtin_delete(std::shared_ptr<bool> armed_flag_ptr)
-        : flag_ptr{armed_flag_ptr} {}
-    template <typename T_                                                         = T,
-              typename std::enable_if<std::is_destructible<T_>::value, int>::type = 0>
-    void operator()(T *raw_ptr) {
-        if (*flag_ptr)
-            delete raw_ptr;
-    }
-    template <typename T_                                                          = T,
-              typename std::enable_if<!std::is_destructible<T_>::value, int>::type = 0>
-    void operator()(T *) {
-        // This noop operator is needed to avoid a compilation error (for `delete raw_ptr;`), but
-        // throwing an exception from here could std::terminate the process. Therefore the runtime
-        // check for lifetime-management correctness is implemented elsewhere (in
-        // ensure_pointee_is_destructible()).
+struct guarded_delete {
+    void (*del_ptr)(void *);
+    bool armed_flag;
+    guarded_delete(void (*del_ptr)(void *), bool armed_flag)
+        : del_ptr{del_ptr}, armed_flag{armed_flag} {}
+    void operator()(void *raw_ptr) const {
+        if (armed_flag)
+            (*del_ptr)(raw_ptr);
     }
 };
 
+template <typename T, typename std::enable_if<std::is_destructible<T>::value, int>::type = 0>
+inline void builtin_delete_if_destructible(void *raw_ptr) {
+    delete (T *) raw_ptr;
+}
+
+template <typename T, typename std::enable_if<!std::is_destructible<T>::value, int>::type = 0>
+inline void builtin_delete_if_destructible(void *) {
+    // This noop operator is needed to avoid a compilation error (for `delete raw_ptr;`), but
+    // throwing an exception from a destructor will std::terminate the process. Therefore the
+    // runtime check for lifetime-management correctness is implemented elsewhere (in
+    // ensure_pointee_is_destructible()).
+}
+
+template <typename T>
+guarded_delete make_guarded_builtin_delete(bool armed_flag) {
+    return guarded_delete(builtin_delete_if_destructible<T>, armed_flag);
+}
+
 template <typename T, typename D>
-struct guarded_custom_deleter {
-    std::shared_ptr<bool> flag_ptr;
-    explicit guarded_custom_deleter(std::shared_ptr<bool> armed_flag_ptr)
-        : flag_ptr{armed_flag_ptr} {}
-    void operator()(T *raw_ptr) {
-        if (*flag_ptr)
-            D()(raw_ptr);
-    }
+inline void custom_delete(void *raw_ptr) {
+    D()((T *) raw_ptr);
+}
+
+template <typename T, typename D>
+guarded_delete make_guarded_custom_deleter(bool armed_flag) {
+    return guarded_delete(custom_delete<T, D>, armed_flag);
 };
 
 template <typename T>
@@ -96,7 +103,6 @@ inline bool is_std_default_delete(const std::type_info &rtti_deleter) {
 
 struct smart_holder {
     const std::type_info *rtti_uqp_del = nullptr;
-    std::shared_ptr<bool> vptr_deleter_armed_flag_ptr;
     std::shared_ptr<void> vptr;
     bool vptr_is_using_noop_deleter : 1;
     bool vptr_is_using_builtin_delete : 1;
@@ -108,20 +114,14 @@ struct smart_holder {
     // Design choice: smart_holder is movable but not copyable.
     smart_holder(smart_holder &&)      = default;
     smart_holder(const smart_holder &) = delete;
-    smart_holder &operator=(smart_holder &&) = default;
+    smart_holder &operator=(smart_holder &&) = delete;
     smart_holder &operator=(const smart_holder &) = delete;
 
     smart_holder()
         : vptr_is_using_noop_deleter{false}, vptr_is_using_builtin_delete{false},
           vptr_is_external_shared_ptr{false}, is_populated{false}, is_disowned{false},
           pointee_depends_on_holder_owner{false} {}
 
-    explicit smart_holder(bool vptr_deleter_armed_flag)
-        : vptr_deleter_armed_flag_ptr{new bool{vptr_deleter_armed_flag}},
-          vptr_is_using_noop_deleter{false}, vptr_is_using_builtin_delete{false},
-          vptr_is_external_shared_ptr{false}, is_populated{false}, is_disowned{false},
-          pointee_depends_on_holder_owner{false} {}
-
     bool has_pointee() const { return vptr.get() != nullptr; }
 
     template <typename T>
@@ -194,10 +194,20 @@ struct smart_holder {
         }
     }
 
+    void reset_vptr_deleter_armed_flag(bool armed_flag) {
+        // The const_cast is only for certain compilers (Ubuntu 20 GCC 6.3.0 being one).
+        auto vptr_del_ptr = const_cast<guarded_delete *>(std::get_deleter<guarded_delete>(vptr));
+        if (vptr_del_ptr == nullptr) {
+            throw std::runtime_error(
+                "smart_holder::reset_vptr_deleter_armed_flag() called in an invalid context.");
+        }
+        vptr_del_ptr->armed_flag = armed_flag;
+    }
+
     template <typename T>
     static smart_holder from_raw_ptr_unowned(T *raw_ptr) {
-        smart_holder hld(false);
-        hld.vptr.reset(raw_ptr, guarded_builtin_delete<T>(hld.vptr_deleter_armed_flag_ptr));
+        smart_holder hld;
+        hld.vptr.reset(raw_ptr, [](void *) {});
         hld.vptr_is_using_noop_deleter = true;
         hld.is_populated               = true;
         return hld;
@@ -227,30 +237,27 @@ struct smart_holder {
     template <typename T>
     static smart_holder from_raw_ptr_take_ownership(T *raw_ptr) {
         ensure_pointee_is_destructible<T>("from_raw_ptr_take_ownership");
-        smart_holder hld(true);
-        hld.vptr.reset(raw_ptr, guarded_builtin_delete<T>(hld.vptr_deleter_armed_flag_ptr));
+        smart_holder hld;
+        hld.vptr.reset(raw_ptr, make_guarded_builtin_delete<T>(true));
         hld.vptr_is_using_builtin_delete = true;
         hld.is_populated                 = true;
         return hld;
     }
 
     // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
     void disown() {
-        *vptr_deleter_armed_flag_ptr = false;
-        is_disowned                  = true;
+        reset_vptr_deleter_armed_flag(false);
+        is_disowned = true;
     }
 
     // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
     void reclaim_disowned() {
-        *vptr_deleter_armed_flag_ptr = true;
-        is_disowned                  = false;
+        reset_vptr_deleter_armed_flag(true);
+        is_disowned = false;
     }
 
     // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
-    void release_disowned() {
-        vptr.reset();
-        vptr_deleter_armed_flag_ptr.reset();
-    }
+    void release_disowned() { vptr.reset(); }
 
     // SMART_HOLDER_WIP: review this function.
     void ensure_can_release_ownership(const char *context = "ensure_can_release_ownership") {
@@ -261,7 +268,7 @@ struct smart_holder {
 
     // Caller is responsible for ensuring preconditions (SMART_HOLDER_WIP: details).
     void release_ownership() {
-        *vptr_deleter_armed_flag_ptr = false;
+        reset_vptr_deleter_armed_flag(false);
         release_disowned();
     }
 
@@ -275,15 +282,13 @@ struct smart_holder {
 
     template <typename T, typename D>
     static smart_holder from_unique_ptr(std::unique_ptr<T, D> &&unq_ptr) {
-        smart_holder hld(true);
+        smart_holder hld;
         hld.rtti_uqp_del                 = &typeid(D);
         hld.vptr_is_using_builtin_delete = is_std_default_delete<T>(*hld.rtti_uqp_del);
         if (hld.vptr_is_using_builtin_delete) {
-            hld.vptr.reset(unq_ptr.get(),
-                           guarded_builtin_delete<T>(hld.vptr_deleter_armed_flag_ptr));
+            hld.vptr.reset(unq_ptr.get(), make_guarded_builtin_delete<T>(true));
         } else {
-            hld.vptr.reset(unq_ptr.get(),
-                           guarded_custom_deleter<T, D>(hld.vptr_deleter_armed_flag_ptr));
+            hld.vptr.reset(unq_ptr.get(), make_guarded_custom_deleter<T, D>(true));
         }
         unq_ptr.release();
         hld.is_populated = true;