From b3050c88c29d238ffa58208328fa32229373e774 Mon Sep 17 00:00:00 2001 From: Danny Robson Date: Thu, 23 Jul 2020 15:16:15 +1000 Subject: [PATCH] signal: simplify the implementation of the cookie and signal --- signal.hpp | 228 +++++++++++++++++++++++++----------------------- test/signal.cpp | 52 +++-------- 2 files changed, 127 insertions(+), 153 deletions(-) diff --git a/signal.hpp b/signal.hpp index ea2375aa..66f8fbf6 100644 --- a/signal.hpp +++ b/signal.hpp @@ -12,114 +12,126 @@ #include "debug/assert.hpp" #include -#include namespace cruft { - namespace reduce { - /// Returns the short-circuited logical-and of the results of all - /// connected callbacks. - struct logical_and { - template - decltype(auto) - operator() (InputT first, InputT last, Args&&... args) - { - while (first != last) - if (!(std::invoke (*first++, args...))) - return false; - return true; - } - }; - - /// Returns the short-circuited logical-or of the results of all - /// connected callbacks. - struct logical_or { - template - 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 - void operator() (InputT first, InputT last, Args&&... args) - { - while (first != last) { - std::invoke (*first++, args...); - } - } - }; - } - + /// A signal object whose clients can listen for invocations. + /// + /// The arguments supplied to the invocation of the parent are supplied + /// to the callback of each child. + /// + /// Every client must store the cookie from `connect`. When this goes out + /// of scope it will disconnect the callback from the signal object. + /// It is permissible to release a cookie while it is being invoked, but + /// no other cookie for the duration. + /// + /// All cookies _should_ be destroyed or released before the signal is + /// destroyed. + /// + /// \tparam FunctionT The type of the callback template < - typename FunctionT, - typename ReductionT = reduce::noop + typename FunctionT > class signal { public: - using reduction_type = ReductionT; using function_type = FunctionT; - typedef std::list 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& operator= (cookie const&) = delete; - - cookie (typename group::iterator _position, - signal &_parent): - m_position (_position), - m_parent (_parent) + cookie (FunctionT &&_callback) + : callback (std::move (_callback)) + , next (nullptr) + , prev (nullptr) { ; } - - cookie (cookie &&rhs) noexcept: - m_position (rhs.m_position), - m_parent (rhs.m_parent) + cookie (cookie &&rhs) noexcept + : cookie (std::move (rhs.callback)) { - rhs.m_position = rhs.m_parent.m_children.end (); + replace (rhs); } cookie& operator= (cookie &&rhs) noexcept { - CHECK_EQ (&m_parent, &rhs.m_parent); - std::swap (m_position, rhs.m_position); + replace (rhs); + 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) - m_parent.disconnect (*this); + release (); } - - void reset (FunctionT &&cb) + /// Take the position of `rhs` in the linked list. + 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; - signal &m_parent; + if (next) { + 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& operator= (signal const&) = default; - - signal (signal &&) noexcept = default; - signal& operator= (signal &&) noexcept = default; + signal (signal const&) = delete; + signal& operator= (signal const&) = delete; + 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 ( - m_children.insert ( - m_children.end (), - std::move (_cb) - ), - *this - ); + cookie res (std::move (_callback)); + m_head.append (res); + return res; } - 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) - { - m_children.clear (); - } + void clear (void); + /// 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 decltype(auto) operator() (ArgsT&&... tail) { - return ReductionT {} ( - m_children.begin (), - m_children.end (), - std::forward (tail)... - ); + // We need to cache the cursor and advance _before_ we invoke + // the cookie so that we have saved a pointer to the next + // child in case it gets released on us during the call. + 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; }; diff --git a/test/signal.cpp b/test/signal.cpp index a3b835ce..699f2df4 100644 --- a/test/signal.cpp +++ b/test/signal.cpp @@ -72,57 +72,26 @@ test_value_signal (cruft::TAP::logger &tap) /////////////////////////////////////////////////////////////////////////////// void -test_combiner (cruft::TAP::logger &tap) -{ - { - cruft::signal, 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, 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) +test_parallel_release (cruft::TAP::logger &tap) { tap.expect_nothrow ([] { using function_t = std::function; cruft::signal sig; - cruft::signal::cookie a = sig.connect ([&] (void) { sig.disconnect (a); }); - cruft::signal::cookie b = sig.connect ([&] (void) { sig.disconnect (b); }); - cruft::signal::cookie c = sig.connect ([&] (void) { sig.disconnect (c); }); - cruft::signal::cookie d = sig.connect ([&] (void) { sig.disconnect (d); }); + cruft::signal::cookie a = sig.connect ([&] (void) { a.release (); }); + cruft::signal::cookie b = sig.connect ([&] (void) { b.release (); }); + cruft::signal::cookie c = sig.connect ([&] (void) { c.release (); }); + cruft::signal::cookie d = sig.connect ([&] (void) { d.release (); }); sig (); - }, "parallel disconnect in invocation"); + }, "parallel release in invocation"); } /////////////////////////////////////////////////////////////////////////////// +#include + + 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_disconnect (tap); + test_parallel_release (tap); }); }