slot_rep: Make destructor, destroy() and dup() virtual

Kjell Ahlstedt · Kjell Ahlstedt · commit a46096555351 · 2017-02-13T19:00:41.000+01:00
* sigc++/functors/slot_base.h:
* sigc++/functors/slot.h: Make ~slot_rep(), slot_rep::destroy() and
slot_rep::dup() virtual. Bug 777618
diff --git a/sigc++/functors/slot.h b/sigc++/functors/slot.h
@@ -43,8 +43,6 @@ template <typename T_functor>
 struct typed_slot_rep : public slot_rep
 {
 private:
-  using self = typed_slot_rep<T_functor>;
-
   /* Use an adaptor type so that arguments can be passed as const references
    * through explicit template instantiation from slot_call#::call_it() */
   using adaptor_type = typename adaptor_trait<T_functor>::adaptor_type;
@@ -58,13 +56,13 @@ struct typed_slot_rep : public slot_rep
    * @param functor The functor contained by the new slot_rep object.
    */
   inline explicit typed_slot_rep(const T_functor& functor)
-  : slot_rep(nullptr, &destroy, &dup), functor_(std::make_unique<adaptor_type>(functor))
+  : slot_rep(nullptr), functor_(std::make_unique<adaptor_type>(functor))
   {
     sigc::visit_each_trackable(slot_do_bind(this), *functor_);
   }
 
   inline typed_slot_rep(const typed_slot_rep& src)
-    : slot_rep(src.call_, &destroy, &dup), functor_(std::make_unique<adaptor_type>(*src.functor_))
+  : slot_rep(src.call_), functor_(std::make_unique<adaptor_type>(*src.functor_))
   {
     sigc::visit_each_trackable(slot_do_bind(this), *functor_);
   }
@@ -74,24 +72,25 @@ struct typed_slot_rep : public slot_rep
   typed_slot_rep(typed_slot_rep&& src) = delete;
   typed_slot_rep& operator=(typed_slot_rep&& src) = delete;
 
-  inline ~typed_slot_rep()
+  ~typed_slot_rep() override
   {
-    call_ = nullptr;
-    destroy_ = nullptr;
-    sigc::visit_each_trackable(slot_do_unbind(this), *functor_);
+    // Call destroy() non-virtually.
+    // It's unwise to make virtual calls in a constructor or destructor.
+    typed_slot_rep::destroy();
   }
 
 private:
   /** Detaches the stored functor from the other referred trackables and destroys it.
    * This does not destroy the base slot_rep object.
    */
-  static void destroy(slot_rep* data)
+  void destroy() override
   {
-    auto self_ = static_cast<self*>(data);
-    self_->call_ = nullptr;
-    self_->destroy_ = nullptr;
-    sigc::visit_each_trackable(slot_do_unbind(self_), *self_->functor_);
-    self_->functor_.reset(nullptr);
+    call_ = nullptr;
+    if (functor_)
+    {
+      sigc::visit_each_trackable(slot_do_unbind(this), *functor_);
+      functor_.reset(nullptr);
+    }
     /* don't call disconnect() here: destroy() is either called
      * a) from the parent itself (in which case disconnect() leads to a segfault) or
      * b) from a parentless slot (in which case disconnect() does nothing)
@@ -103,7 +102,7 @@ struct typed_slot_rep : public slot_rep
    * slot_rep object is registered in the referred trackables.
    * @return A deep copy of the slot_rep object.
    */
-  static slot_rep* dup(slot_rep* a_rep) { return new self(*static_cast<self*>(a_rep)); }
+  slot_rep* dup() const override { return new typed_slot_rep(*this); }
 };
 
 /** Abstracts functor execution.
diff --git a/sigc++/functors/slot_base.h b/sigc++/functors/slot_base.h
@@ -66,20 +66,14 @@ struct SIGC_API slot_rep : public trackable
    * down dereferencing of slot list iterators. Martin. */
   // TODO: Try this now? murrayc.
 
-  using hook_dup = slot_rep* (*)(slot_rep*);
-
-  using func_slot_rep_destroy_notify = void (*)(slot_rep* data);
-
-  inline slot_rep(hook call__, func_slot_rep_destroy_notify destroy__, hook_dup dup__) noexcept
+  inline slot_rep(hook call__) noexcept
     : call_(call__),
       cleanup_(nullptr),
-      parent_(nullptr),
-      destroy_(destroy__),
-      dup_(dup__)
+      parent_(nullptr)
   {
   }
 
-  inline ~slot_rep() { destroy(); }
+  virtual ~slot_rep() {}
 
 // only MSVC needs this to guarantee that all new/delete are executed from the DLL module
 #ifdef SIGC_NEW_DELETE_IN_LIBRARY_ONLY
@@ -89,19 +83,12 @@ struct SIGC_API slot_rep : public trackable
 
   /** Destroys the slot_rep object (but doesn't delete it).
    */
-  inline void destroy()
-  {
-    if (destroy_)
-      (*destroy_)(this);
-  }
+  virtual void destroy() = 0;
 
   /** Makes a deep copy of the slot_rep object.
    * @return A deep copy of the slot_rep object.
    */
-  inline slot_rep* dup() const
-  {
-    return (*dup_)(const_cast<slot_rep*>(this));
-  }
+  virtual slot_rep* dup() const = 0;
 
   /** Set the parent with a callback.
    * slots have one parent exclusively.
@@ -148,19 +135,6 @@ struct SIGC_API slot_rep : public trackable
 
   /** Parent object whose callback cleanup_ is executed on notification. */
   notifiable* parent_;
-
-protected:
-  /// Callback that detaches the slot_rep object from referred trackables and destroys it.
-  /* This could be a replaced by a virtual dtor. However, since this struct is
-   * crucial for the efficiency of the whole library, we want to avoid this.
-   */
-  func_slot_rep_destroy_notify destroy_;
-
-private:
-  /** Callback that makes a deep copy of the slot_rep object.
-   * @return A deep copy of the slot_rep object.
-   */
-  hook_dup dup_;
 };
 
 /** Functor used to add a dependency to a trackable.
