8
\$\begingroup\$

This is a follow-up to this question, which resulted in major confusion on my side regarding forwarding of arguments. Advice regarding that should probably be given in my question on SO.

Overview

The classes below implement the observer pattern for use in an embedded system. My main need is to be able to connect sources of data (a GPS with position information, for example) to consumers (debugging over a serial line or a position widget). A Signal may have arguments, which are sent to the observers (zero or more Connections), which call a delegate (free function, static or non-static member).

The return values of any called function are discarded in the interface of AbstractDelegate, because the calling Signal shouldn't need to care about any return values. I didn't use std::function for this because it simply occupied too much the system`s available RAM.

Signals can be copied, because they would be used as object members. A class that contains a Signal member should not be non-copyable just because a Signal is present. I'm not sure if I want to support Connection members, please enlighten me if you have good advice about that.

Connections are created on the heap using two connect functions - one for free functions and static methods, one for non-static methods.

I didn't use an STL container for the Signal's connection list because I need control over memory allocation, and a Connection must be able to remove itself from the Signal's list. The result of these two (maybe wrong) thoughts is the intrusive linked list I implemented.

Signals can be blocked (block(), unblock()). If that is the case, no Connection will be notified. Connections can also be blocked individually.

The header

#ifndef SIGNALS_H
#define SIGNALS_H

#include <utility>

/** Interface for delegates with a specific set of arguments **/
template<typename... args>
class AbstractDelegate
{
  public:
    virtual void operator()(args...) const = 0;
    virtual ~AbstractDelegate() {}
};

/** Concrete member function delegate that discards the function's return value **/
template<typename T, typename ReturnType, typename... args>
class ObjDelegate : public AbstractDelegate<args...>
{
  public:
    /** member function typedef **/
    using ObjMemFn = ReturnType (T::*)(args...);

    /** constructor **/
    ObjDelegate(T& obj, ObjMemFn memFn)
      : obj_(obj), // brace-enclosed initializer list didn't work here
      memFn_{memFn} // here the brace-enclosed list works, probably because memFn is _not_ a reference
    {
    }

    /** call operator that calls the stored function on the stored object **/
    void operator()(args... a) const override
    {
      (obj_.*memFn_)(std::forward<args>(a)...);
    }

  private:
    /** reference to the object **/
    T& obj_;
    /** member function pointer **/
    const ObjMemFn memFn_;
};

/** Concrete function delegate that discards the function's return value **/
template<typename ReturnType, typename... args>
class FnDelegate : public AbstractDelegate<args...>
{
  public:
    /** member function typedef **/
    using Fn = ReturnType(*)(args...);

    /** constructor **/
    FnDelegate(Fn fn)
      : fn_{fn}
    {
    }

    /** call operator that calls the stored function **/
    void operator()(args... a) const override
    {
      (*fn_)(std::forward<args>(a)...);
    }

  private:
    /** function pointer **/
    const Fn fn_;
};

/** forward declaration **/
template<typename... args>
class Connection;

/** Signal class that can be connected to**/
template<typename... args>
class Signal
{
  public:
    /** connection pointer typedef **/
    using connection_p = Connection<args...>*;

    /** constructor **/
    Signal()
      : connections_(nullptr),
      blocked_(false)
      {
      }

    /** copy constructor **/
    Signal(const Signal& other)
      : connections_(nullptr),
      blocked_(other.blocked()) // not sure if this is a good idea
      {
      }

    /** call operator that notifes all connections associated with this Signal.
The most recently associated connection will be notified first **/
    void operator()(args... a) const
    {
      // only notify connections if this signal is not blocked
      if (!blocked())
      {
        auto c = connections_;
        while(c)
        {
          auto c_next = c->next();
          if (c_next)
            (*c)(a...);
          else
            (*c)(std::forward<args>(a)...); // last use, can forward
          c = c_next;
        }
      }
    }

    /** connect to this signal **/
    void connect(connection_p p)
    {
      p->next_ = connections_;
      connections_ = p;
      p->signal_ = this;
    }

    /** disconnect from this signal.
Invalidates the connection's signal pointer
and removes the connection from the list **/
    void disconnect(connection_p conn)
    {
      // find connection and remove it from the list
      connection_p c = connections_;
      if (c == conn)
      {
        connections_ = connections_->next();
        conn->next_ = nullptr;
        conn->signal_ = nullptr;
        return;
      }
      while(c != nullptr)
      {
        if (c->next() == conn)
        {
          c->next_ = conn->next();
          conn->next_ = nullptr;
          conn->signal_ = nullptr;
          return;
        }
        c = c->next();
      }
    }

    /** block events from this signal **/
    void block()
    {
      blocked_ = true;
    }

    /** unblock events from this signal **/
    void unblock()
    {
      blocked_ = false;
    }

    /** is this signal blocked? **/
    bool blocked() const
    {
      return blocked_;
    }

    /** destructor. disconnects all connections **/
    ~Signal()
    {
      connection_p p = connections_;
      while(p != nullptr)
      {
        connection_p n = p->next();
        disconnect(p);
        p = n;
      }
    }

    connection_p connections() const {return connections_;}

  private:
    /** don't allow copy assignment **/
    Signal& operator= (Signal& other);

    connection_p connections_;
    bool blocked_;
};

/** connection class that can be connected to a signal **/
template<typename... args>
class Connection
{
  public:
    /** template constructor for non-static member functions.
allocates a new delegate on the heap **/
    template<typename T, typename ReturnType>
    Connection(Signal<args...>& signal, T& obj, ReturnType (T::*memFn)(args...))
      : delegate_(new ObjDelegate<T, ReturnType, args...>(obj, memFn)),
      signal_(nullptr),
      next_(nullptr),
      blocked_(false)
    {
      signal.connect(this);
    }

    /** template constructor for static member functions and free functions.
allocates a new delegate on the heap **/
    template<typename ReturnType>
    Connection(Signal<args...>& signal, ReturnType (*Fn)(args...))
      : delegate_(new FnDelegate<ReturnType, args...>(Fn)),
      signal_(nullptr),
      next_(nullptr),
      blocked_(false)
    {
      signal.connect(this);
    }

    /** get reference to this connection's delegate **/
    AbstractDelegate<args...>& delegate() const
    {
      return *delegate_;
    }

    /** call this connection's delegate if not blocked **/
    void operator()(args... a) const
    {
      if (!blocked())
      {
        delegate()(std::forward<args>(a)...);
      }
    }

    /** get pointer to next connection in the signal's list **/
    Connection* next() const
    {
      return next_;
    }

    /** is this connection connected to a valid signal? **/
    bool connected() const
    {
      return (signal_ != nullptr);
    }

    /** block events for this connection **/
    void block()
    {
      blocked_ = true;
    }

    /** unblock events for this connection **/
    void unblock()
    {
      blocked_ = false;
    }

    /** is this connection blocked? **/
    bool blocked() const
    {
      return blocked_;
    }

    /** desctructor. If the signal is still alive, disconnects from it **/
    ~Connection()
    {
      if (signal_ != nullptr)
      {
        signal_->disconnect(this);
      }
      delete delegate_;
    }

    const Signal<args...>* signal() const {return signal_;}

    friend class Signal<args...>;
  private:
    /** don't allow copy construction **/
    Connection(const Connection& other);

    /** don't allow copy assignment **/
    Connection& operator= (Connection& other);

    AbstractDelegate<args...>* delegate_;
    Signal<args...>* signal_;
    Connection* next_;
    bool blocked_;
};

/** free connect function: creates a connection (non-static member function) on the heap
that can be used anonymously **/
template<typename T, typename ReturnType, typename... args>
Connection<args...>* connect(Signal<args...>& signal, T& obj, ReturnType (T::*memFn)(args...))
{
  return new Connection<args...>(signal, obj, memFn);
}

/** free connect function: creates a connection (static member or free function) on the heap
that can be used anonymously **/
template<typename ReturnType, typename... args>
Connection<args...>* connect(Signal<args...>& signal, ReturnType (*fn)(args...))
{
  return new Connection<args...>(signal, fn);
}

#endif // SIGNALS_H

And how it's supposed to be used

#include <iostream>
#include "signals.h"

Signal<int> sig; // create a signal

void print(int i) // free function to print an int
{
  std::cout << "print(" << i << ")" << std::endl;
}

int get(int i) // a function that returns an int
{
  return i;
}

class Foo
{
  public:
    Foo(const int& i)
     : i_(i)
    {
      std::cout << "Foo(" << i << ")" << std::endl;
    }
    const int& operator()() const {return i_;}
    void addAndPrint(int i)
    {
        std::cout << "Foo::addAndPrint(" << i + i_ << ")" << std::endl;
    }
  private:
    int i_;
};

int main()
{
  connect(sig, print); // when sig is called, print() should be notified
  sig(3);
  int i = 4;
  sig(i);
  sig(get(5));

  sig(Foo(6)());
  Foo foo(8);
  sig(foo());

  connect(sig, foo, &Foo::addAndPrint);
  sig(10);

}

My main concerns are about copy construction and assignment of Signals and Connections. I added those operations because I needed destructors for those classes (rule of three) - taking a look at the desctructors might also be a good idea.

I'm also interested in pitfalls I might have overlooked. I think I have made sure that deleting a Signal or a Connection does not leave any dangling pointers, but that might not be the only thing that can go wrong here.

\$\endgroup\$
6
+50
\$\begingroup\$

I will try not to give awful advice like I did last time. Here are a few remarks:

  • First of all, let's come back to std::forward. You should follow the link and read again a little bit more; std::forward is only useful for reference collapsing. In your operator(), you take the arguments by copy (that is fine) and there is no reference collapsing. Therefore, where you use std::forward, you should use std::move instead (from the header <algorithm>), following the copy-then-move idiom.

  • You can simplify your default constructor. First of all, you can use in-class initializers to initialize connections_ and blocked_:

    connection_p connections_ = nullptr;
    bool blocked_             = false;
    

    Now, if the values to pass to connections_ and blocked_ are not specified in a constructor, the compiler will use the values from the in-class initializers instead. That means that you can reduce the implementation of Signal's default constructor to:

    Signal() = default;
    
  • There are too many comments. Try to remove the unneeded comments. Many of your comments do not give more information that what we already know by reading the corresponding line of code. Even worse, if you modify the code and not the comments, they will live. Try to writer self-documenting code to avoid unneeded and potentially liar comments.

  • However, your comments helped me figure out that you need a C++11 feature here:

    private:
        /** don't allow copy construction **/
        Connection(const Connection& other);
    
        /** don't allow copy assignment **/
        Connection& operator= (Connection& other);
    

    Instead of making these special functions private, you can explicitely mark them as deleted. In other words, you can write this instead:

    Connection(const Connection& other)
        = delete;
    Connection& operator= (Connection& other)
        = delete;
    

    And the same holds for copy assignment operator of Signal.

  • Thanks @Jamal for this one: try not to use std::endl when it is not needed. Actually, std::endl adds a newline character and flushes the output buffer. You generally don't need the buffer to be flushed, you only want to add a newline character. Moreover, the buffer is automatically flushed when the program ends.

  • There are many places where you can still use list initialization to avoid potential implicit narrowing conversion. I already mentioned that in my answer to your previous question and forgot to mention the cases where it was not possible (references for example). A better guideline would actually be to use list initialization when you use numbers (integers of floating point numbers) and you know that a loss of precision could be a problem. You can also use it to prevent implicit conversions to bool, which sometimes don't make sense.

\$\endgroup\$
  • \$\begingroup\$ It's been a while since I asked this question and I could finally try out your suggestions. The delete keyword and in-class initializers were new to me and they certainly simplify things. Thank you! \$\endgroup\$ – Christoph May 23 '14 at 23:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.