signal: simplify the implementation of the cookie and signal

signal.hpp

@@ -12,114 +12,126 @@
 #include "debug/assert.hpp"
 
 #include <functional>
-#include <list>
 
 
 namespace cruft {
-    namespace reduce {
+    /// A signal object whose clients can listen for invocations.
-        /// Returns the short-circuited logical-and of the results of all
+    ///
-        /// connected callbacks.
+    /// The arguments supplied to the invocation of the parent are supplied
-        struct logical_and {
+    /// to the callback of each child.
-            template <typename InputT, typename ...Args>
+    ///
-            decltype(auto)
+    /// Every client must store the cookie from `connect`. When this goes out
-            operator() (InputT first, InputT last, Args&&... args)
+    /// of scope it will disconnect the callback from the signal object.
-            {
+    /// It is permissible to release a cookie while it is being invoked, but
-                while (first != last)
+    /// no other cookie for the duration.
-                    if (!(std::invoke (*first++, args...)))
+    ///
-                        return false;
+    /// All cookies _should_ be destroyed or released before the signal is
-                return true;
+    /// destroyed.
-            }
+    ///
-        };
+    /// \tparam FunctionT   The type of the callback
-
-        /// Returns the short-circuited logical-or of the results of all
-        /// connected callbacks.
-        struct logical_or {
-            template <typename InputT, typename ...Args>
-            decltype(auto)
-            operator() (InputT first, InputT last, Args&&... args)
-            {
-                while (first != last)
-                    if (std::invoke (*first++, args...))
-                        return true;
-                return false;
-            }
-        };
-
-        /// Unconditionally evaluates all connected callbacks. Returns nothing.
-        struct noop {
-            template <typename InputT, typename ...Args>
-            void operator() (InputT first, InputT last, Args&&... args)
-            {
-                while (first != last) {
-                    std::invoke (*first++, args...);
-                }
-            }
-        };
-    }
-
     template <
-        typename FunctionT,
+        typename FunctionT
-        typename ReductionT = reduce::noop
     >
     class signal {
         public:
-            using reduction_type = ReductionT;
             using function_type = FunctionT;
 
-            typedef std::list<FunctionT> group;
-
             ///////////////////////////////////////////////////////////////////////
+            /// A single node in a doubly linked list of callbacks to invoke when
+            /// a signal is invoked.
+            ///
+            /// They perform no direct processing by themselves. Instead they are
+            /// entirely focused on managing lifetimes of, and pointers to, the
+            /// callbacks that will be invoked.
             struct cookie {
-                cookie (cookie const&) = delete;
+                cookie (FunctionT &&_callback)
-                cookie& operator= (cookie const&) = delete;
+                    : callback (std::move (_callback))
-
+                    , next (nullptr)
-                cookie (typename group::iterator _position,
+                    , prev (nullptr)
-                        signal<FunctionT,ReductionT> &_parent):
-                    m_position (_position),
-                    m_parent (_parent)
                 { ; }
 
-
+                cookie (cookie &&rhs) noexcept
-                cookie (cookie &&rhs) noexcept:
+                    : cookie (std::move (rhs.callback))
-                    m_position (rhs.m_position),
-                    m_parent   (rhs.m_parent)
                 {
-                    rhs.m_position = rhs.m_parent.m_children.end ();
+                    replace (rhs);
                 }
 
                 cookie& operator= (cookie &&rhs) noexcept
                 {
-                    CHECK_EQ (&m_parent, &rhs.m_parent);
+                    replace (rhs);
-                    std::swap (m_position, rhs.m_position);
+                    callback = std::move (rhs.callback);
                     return *this;
                 }
 
+                cookie (cookie const&) = delete;
+                cookie& operator= (cookie const&) = delete;
+
                 ~cookie ()
                 {
-                    if (m_parent.m_children.end () != m_position)
+                    release ();
-                        m_parent.disconnect (*this);
                 }
 
-
+                /// Take the position of `rhs` in the linked list.
-                void reset (FunctionT &&cb)
+                void replace (cookie &rhs)
                 {
-                    *m_position = std::move (cb);
+                    // It could be more efficient to special case these
+                    // operations but this shouldn't be an operation that
+                    // occurs in a hot loop anyway.
+                    release ();
+                    rhs.append (*this);
+                    rhs.release ();
                 }
 
+                /// Link the provided cookie into the linked list as our
+                /// successor.
+                ///
+                /// The provided cookie must not currently be part of any
+                /// linked list.
+                void append (cookie &rhs)
+                {
+                    CHECK (rhs.next == nullptr);
+                    CHECK (rhs.prev == nullptr);
 
-                typename group::iterator m_position;
+                    if (next) {
-                signal<FunctionT,ReductionT> &m_parent;
+                        CHECK_EQ (next->prev, this);
+                        next->prev = &rhs;
+                    }
+
+                    rhs.next = next;
+                    rhs.prev = this;
+
+                    next = &rhs;
+                }
+
+                /// If this cookie is part of a linked list then remove it
+                /// from the linked list. Otherwise this is a noop.
+                void release (void)
+                {
+                    if (next) next->prev = prev;
+                    if (prev) prev->next = next;
+
+                    next = nullptr;
+                    prev = nullptr;
+                }
+
+                FunctionT callback;
+
+                /// The next node in the linked list. Or nullptr if this is the end.
+                cookie *next;
+                /// The previous node in the linked list. Or nullptr if this is the start.
+                cookie *prev;
             };
 
         public:
-            signal () = default;
+            signal ()
+                : m_head (FunctionT{})
+            { ; }
 
-            signal (signal const&) = default;
+            signal (signal const&) = delete;
-            signal& operator= (signal const&) = default;
+            signal& operator= (signal const&) = delete;
-
-            signal (signal &&) noexcept = default;
-            signal& operator= (signal &&) noexcept = default;
 
+            signal (signal &&rhs) noexcept = default;
+            signal& operator= (signal &&rhs) noexcept = default;
 
             ~signal ()
             {
@@ -127,65 +139,59 @@ namespace cruft {
             }
 
             /// Add a callback to list.
-            cookie connect [[nodiscard]] (FunctionT &&_cb)
+            cookie
+            connect [[nodiscard]] (FunctionT &&_callback)
             {
-                return cookie (
+                cookie res (std::move (_callback));
-                    m_children.insert (
+                m_head.append (res);
-                        m_children.end (),
+                return res;
-                        std::move (_cb)
-                    ),
-                    *this
-                );
             }
 
-            cookie connect [[nodiscard]] (FunctionT const &_cb)
-            {
-                return cookie (
-                    m_children.insert (
-                        m_children.end (),
-                        std::move (_cb)
-                    ),
-                    *this
-                );
-            }
-
-            void disconnect (cookie &c)
-            {
-                m_children.erase (c.m_position);
-                c.m_position = m_children.end ();
-            }
 
             /// Disconnect all callbacks
-            void clear (void)
+            void clear (void);
-            {
+
-                m_children.clear ();
-            }
 
             /// Returns the number of callbacks connected.
-            size_t size (void) const
+            std::size_t size (void) const
             {
-                return m_children.size ();
+                std::size_t accum = 0;
+                for (auto cursor = m_head.next; cursor; cursor = cursor->next)
+                    ++accum;
+                return accum;
             }
 
+
             bool empty (void) const
             {
-                return m_children.empty ();
+                return m_head.next == nullptr;
             }
 
+
             /// Execute all callbacks
             template <typename ...ArgsT>
             decltype(auto)
             operator() (ArgsT&&... tail)
             {
-                return ReductionT {} (
+                // We need to cache the cursor and advance _before_ we invoke
-                    m_children.begin (),
+                // the cookie so that we have saved a pointer to the next
-                    m_children.end (),
+                // child in case it gets released on us during the call.
-                    std::forward<ArgsT> (tail)...
+                cookie *cursor = m_head.next;
-                );
+
+                while (cursor) {
+                    auto now = cursor;
+                    cursor = cursor->next;
+                    std::invoke (now->callback, tail...);
+                }
             }
 
+
         private:
-            group m_children;
+            /// The first node in the linked list of callbacks. We
+            /// unconditionally store this node to simplify anchoring the
+            /// linked list to the signal object and removing nodes via their
+            /// destructors.
+            cookie m_head;
     };
 
 

test/signal.cpp

@@ -72,57 +72,26 @@ test_value_signal (cruft::TAP::logger &tap)
 
 ///////////////////////////////////////////////////////////////////////////////
 void
-test_combiner (cruft::TAP::logger &tap)
+test_parallel_release (cruft::TAP::logger &tap)
-{
-    {
-        cruft::signal<std::function<bool(void)>, cruft::reduce::logical_and> sig;
-
-        unsigned count = 0;
-        auto cookie0 = sig.connect ([&] (void) { ++count; return true; });
-        auto cookie1 = sig.connect ([&] (void) { ++count; return true; });
-        auto cookie2 = sig.connect ([&] (void) { ++count; return true; });
-
-        tap.expect (sig (), "bool signal, success");
-        tap.expect_eq (count, 3u, "bool signal, success, count");
-    }
-
-    {
-        cruft::signal<std::function<bool(void)>, cruft::reduce::logical_and> sig;
-
-        unsigned count = 0;
-        auto cookie0 = sig.connect ([&] (void) { ++count; return true; });
-        auto cookie1 = sig.connect ([&] (void) { ++count; return false; });
-        auto cookie2 = sig.connect ([&] (void) { ++count; return true; });
-
-        tap.expect (!sig (), "bool signal, failure");
-
-        // ordering of signals is not guaranteed so we can't say for sure how
-        // many callbacks will be triggered; it will _probably_ be in order
-        // though.
-        tap.expect_le (count, 3u, "bool signal, failure, count");
-    }
-}
-
-
-///////////////////////////////////////////////////////////////////////////////
-void
-test_disconnect (cruft::TAP::logger &tap)
 {
     tap.expect_nothrow ([] {
         using function_t = std::function<void(void)>;
         cruft::signal<function_t> sig;
 
-        cruft::signal<function_t>::cookie a = sig.connect ([&] (void) { sig.disconnect (a); });
+        cruft::signal<function_t>::cookie a = sig.connect ([&] (void) { a.release (); });
-        cruft::signal<function_t>::cookie b = sig.connect ([&] (void) { sig.disconnect (b); });
+        cruft::signal<function_t>::cookie b = sig.connect ([&] (void) { b.release (); });
-        cruft::signal<function_t>::cookie c = sig.connect ([&] (void) { sig.disconnect (c); });
+        cruft::signal<function_t>::cookie c = sig.connect ([&] (void) { c.release (); });
-        cruft::signal<function_t>::cookie d = sig.connect ([&] (void) { sig.disconnect (d); });
+        cruft::signal<function_t>::cookie d = sig.connect ([&] (void) { d.release (); });
 
         sig ();
-    }, "parallel disconnect in invocation");
+    }, "parallel release in invocation");
 }
 
 
 ///////////////////////////////////////////////////////////////////////////////
+#include <iostream>
+
+
 int
 main (int, char **)
 {
@@ -131,7 +100,6 @@ main (int, char **)
         test_single (tap);
         test_double (tap);
         test_value_signal (tap);
-        test_combiner (tap);
+        test_parallel_release (tap);
-        test_disconnect (tap);
     });
 }