4
\$\begingroup\$

I am currently working on a game and found myself in need of an event handler. I wrote an event handler similar to this one some time ago, but decided to update it using variadic templates (this is my first time using them). I really wanted something with the simple usage that C# events provide. I found some examples that emulate that usage very well, but were a bit more complex than I'd like.

If anyone has any tips for improving this header-only event handling system, or spots any glaring flaws, I would really appreciate it.

#ifndef _EVENT_HANDLER_H_
#define _EVENT_HANDLER_H_

#include <list>

namespace Utilities
{
    template <typename... Args>
    class Event;

    ///////////////////////////////////////////////////////////////////////////////////////////
    // DelegateContainer
    ///////////////////////////////////////////////////////////////////////////////////////////
    template <typename... Args>
    class DelegateContainer
    {
        friend class Event<Args...>;

    protected:
        std::list<Event<Args...>*> _subscriptions;

    public:
        DelegateContainer() { }

        DelegateContainer(const DelegateContainer& d)
        { _subscriptions.assign(d._subscriptions.begin(), d._subscriptions.end()); }

        DelegateContainer(DelegateContainer&& d)
        {
            _subscriptions.assign(d._subscriptions.begin(), d._subscriptions.end());
            d._subscriptions.clear();
        }

        ~DelegateContainer();

        DelegateContainer& operator=(const DelegateContainer& d)
        {
            _subscriptions.assign(d._subscriptions.begin(), d._subscriptions.end());
            return *this;
        }//operator=

        DelegateContainer& operator=(DelegateContainer&& d)
        {
            _subscriptions.assign(d._subscriptions.begin(), d._subscriptions.end());
            d._subscriptions.clear();
            return *this;
        }//operator=

        virtual bool operator()(Args...) = 0;
    };

    ///////////////////////////////////////////////////////////////////////////////////////////
    // Delegate
    ///////////////////////////////////////////////////////////////////////////////////////////
    template <typename T, typename... Args>
    class Delegate final : public DelegateContainer<Args...>
    {
    private:
        T* _t;
        void (T::*_callback)(Args...);

    public:
        Delegate() : _t(0),
                     _callback(0)
        { }

        Delegate(T* t, void(T::*callback)(Args...)) : _t(t),
                                                      _callback(callback)
        { }

        Delegate(const Delegate& d) : _t(d._t),
                                      _callback(d._callback)
        { }

        Delegate(Delegate&& d) : _t(d._t),
                                 _callback(d._callback)
        {
            _subscriptions.assign(d._subscriptions.begin(), d._subscriptions.end());
            d._subscriptions.clear();
        }

        ~Delegate() { }

        Delegate& operator=(const Delegate& d)
        {
            _subscriptions.assign(d._subscriptions.begin(), d._subscriptions.end());
            _callback = d._callback;
            _t = d._t;

            return *this;
        }//operator=

        Delegate& operator=(Delegate&& d)
        {
            _subscriptions.assign(d._subscriptions.begin(), d._subscriptions.end());
            d._subscriptions.clear();
            _callback = d._callback;
            _t = d._t;

            return *this;
        }//operator=

    public:
        bool operator()(Args... args)
        {
            if (_t && _callback)
            {
                ((*_t).*(_callback))(args...);
                return true;
            }
            return false;
        }//operator()
    };

    ///////////////////////////////////////////////////////////////////////////////////////////
    // Event
    ///////////////////////////////////////////////////////////////////////////////////////////
    template <typename... Args>
    class Event
    {
    private:
        std::list<DelegateContainer<Args...>*> _delegates;

    public:
        Event() { }

        Event(const Event& e)
        { _delegates.assign(e._delegates.begin(), e._delegates.end()); }

        Event(Event&& e)
        {
            _delegates.assign(e._delegates.begin(), e._delegates.end());
            e._delegates.clear();
        }

        ~Event()
        {
            auto itr = _delegates.begin();
            while(itr != _delegates.end())
                (*itr++)->_subscriptions.remove(this);
        }

        Event& operator=(const Event& e)
        {
            _delegates.assign(e._delegates.begin(), e._delegates.end());
            return *this;
        }//operator=

        Event& operator=(Event&& e)
        {
            _delegates.assign(e._delegates.begin(), e._delegates.end());
            e._delegates.clear();
            return *this;
        }//operator=

        Event& operator+=(DelegateContainer<Args...>& d)
        {
            d._subscriptions.push_back(this);
            _delegates.push_back(&d);
            return *this;
        }//operator+=

        Event& operator-=(DelegateContainer<Args...>& d)
        {
            d._subscriptions.remove(this);
            _delegates.remove(&d);
            return *this;
        }//operator-=

        void operator()(Args... args)
        {
            auto itr = _delegates.begin();
            while(itr != _delegates.end())
                if ((*itr)->operator()(args...))
                    itr++;
                else
                    itr = _delegates.erase(itr);
        }//operator()
    };

    ///////////////////////////////////////////////////////////////////////////////////////////
    // DelegateContainer destructor
    ///////////////////////////////////////////////////////////////////////////////////////////
    template <typename... Args>
    DelegateContainer<Args...>::~DelegateContainer()
    {       
        auto itr = _subscriptions.begin();
        while(itr != _subscriptions.end())
            (*itr++)->operator-=(*this);
    }
}//Utilities

#endif // _EVENT_HANDLER_H_

The DelgateContainer is used as a base for Delegate so the Event can call Delegates without any knowledge of the object the Delegate belongs to.

~DelegateContainer() is defined at the end because it needs to be able to unsubscribe from any Events it is subscribed to before destruction.

Use case:

#include <iostream>

#include "EventHandler.h"

using namespace Utilities;

class Activator
{
public:
    Event<int, char> _myEvent;

    Activator() { }

    void Update()
    {
        _myEvent(1, 'A');
        std::cout << std::endl;
    }//Update
};

class Listener
{
public:
    int _i;
    Delegate<Listener, int, char> _myDelegate;

    Listener(int i) : _i(i)
    {
        _myDelegate = Delegate<Listener, int, char>(this, &Listener::DelegateMethod);
    }

    void DelegateMethod(int i, char c)
    {
        std::cout << ' ' << i * _i << ' ' << c << std::endl;
    }//DelegateMethod
};

int main(int argc, char* argv[])
{
    Activator activator;
    Listener listener0 = Listener(0);
    Listener listener1 = Listener(1);
    Listener* listener2 = new Listener(2);

    activator._myEvent += listener0._myDelegate;
    activator._myEvent += listener1._myDelegate;
    activator._myEvent += listener2->_myDelegate;
    activator.Update();

    activator._myEvent -= listener0._myDelegate;
    activator.Update();

    activator._myEvent += listener0._myDelegate;
    activator.Update();

    delete listener2;
    activator.Update();

    return 0;
}

The use case has the following output:

0 A
1 A
2 A

1 A
2 A

1 A
2 A
0 A

1 A
0 A
\$\endgroup\$
0

1 Answer 1

Reset to default
5
\$\begingroup\$

In no particular order:

  1. Normally, you shouldn't define your own copy and move constructors and assignment operators unless you need special processing in them. Which in your case you don't; the implicitly generated ones will do exactly what you need. So just remove them and with them, a potential source of bugs or inefficiencies.

    Unfortunately, this doesn't hold if you're also targetting Visual Studio and need efficient moves, because their compiler does not generate implicit move constructors and move assignment operators (even 2013 won't do that :-( ). So if you're targetting VS as well, you'll have to provide them. I will assume you do, and comment on the ones you have.

    In constructors, you should prefer initialising to assignment or modification, and you should re-use existing functionality whenever possible. So the copy & move constructors for DelegateContainer, for example, would be better written like this:

    DelegateContainer(const DelegateContainer& d)
  : _subscriptions(d._subscriptions)
{}

DelegateContainer(DelegateContainer&& d)
  : _subscriptions(std::move(d._subscriptions))
{}

    The same notes on reuse hold for the assignment operators as well:

    DelegateContainer& operator= (const DelegateContainer& d)
{
  _subscriptions = d._subscriptions;
  return *this;
}

DelegateContainer& operator= (DelegateContainer&& d)
{
  _subscriptions = std::move(d._subscriptions);
  return *this;
}

    It's doubly important to do proper reuse in derived classes. For example, you've forgotten to copy the subscriptions in Delegate's copy constructor. So simply do this instead:

    Delegate(const Delegate& d)
  : DelegateContainer<Args...>(d)
  , _t(d._t)
  , _callback(d._callback)
{}

Delegate(Delegate&& d)
  : DelegateContainer<Args...>(std::move(d))
  , _t(std::move(d._t))
  , _callback(std::move(d._callback))
{}

Delegate& operator= (const Delegate& d)
{
  DelegateContainer<Args...>::operator= (d);
  _t = d._t;
  _callback = d._callback;
  return *this;
}

Delegate& operator= (Delegate&& d)
{
  DelegateContainer<Args...>::operator= (std::move(d));
  _t = std::move(d._t);
  _callback = std::move(d._callback);
  return *this;
}

    This analogously applies to copy and move members of Event as well, of course. As an alterantive for basically duplicating the constructor functionality in the assignment operators, you can look at the copy and swap idiom.

    Again, you only have to do this if you're targeting Visual Studio. Otherwise, just remove all of those member functions and the compiler will generate precisely the above code for you.

    There is also the question whether you actually need the efficiency of moves. It might turn out it's not such a performance hit (how often are you going to copy delegates?) and the reduced maintenance could offset that, so you could decide to drop the copy and move members altogether and simply accept the sub-optimal performance under VS.

  2. Readability of Delegate::operator() could be improved. And you should use forwarding inside, otherwise the call will fail if any callback has a parameter of r-value reference type.

    void operator() (Args... args)
{
  if (_t && _callback)
  {
    (_t->*_callback)(std::forward<Args>(args)...);
    return true;
  }
  return false;
}

    The same comments on forwarding apply to Event::operator() as well, of course.

  3. The destructor of DelegateContainer has a bug. If someone registers the same delegate twice with the same event, it could happen that Event::operator-= will invalidate both the old value of iter and the new one, which means you'd be dereferencing an invalid iterator in the next iteration.

    Generally, it's a bad idea to modify a sequence you're iterating over. Instead, implement the destructor like this:

    template <typename... Args>
DelegateContainer::~DelegateContainer()
{
  while (!_subscriptions.empty())
    *_subscriptions.front() -= *this;
}

  4. I can't see how you handle pointer issues when copying/moving events. When an event is copied, the copy gets the original's list of delegates, but these delegates are not notified of also being registered with the event's copy. Consider this code:

    auto* d1 = new Delegate(/*some initialisation*/);
auto* e1 = new Event;

*e1 += *d1;
  //now d1->_subscriptions is [e1]
  //and e1->_delegates is [d1]
auto* e2 = new Event(*e1);
  //now d1->_subscriptions is [e1]
  //and e1->_delegates is [d1]
  //and e2->_delegates is [d1]
delete d1;
  //Removes d1 from all events in d1->_subscriptions
  //So e1->_delegates is []
  //But e2->_delegates is still [d1]
(*e2)();  //whoops, dereference-after-free of d1!

    The same holds for retargetting pointers when you move an event as well, of course.

  5. Your include guard macro is illegal. Identifiers which start with an underscore followed by an uppercase letter (as well as those containing two consecutive underscores anywhere) are reserved for the compiler & standard library. You're not allowed to use such identifiers in your code.

  6. I understand the usage code is for demonstation only, but you should prefer the mem-initialiser list to assignment even there:

        Listener(int i) : _i(i), _myDelegate(this, &Listener::DelegateMethod)
    {}

  7. You could simplify the entire scenario with a slight shift in design: get rid of Delegate, move its funcitonality into DelegateContainer, and use std::function<void (Args...)> instead of an object pointer and pointer-to-member-function. You can the implement operator() directly in DelegateContainer, as it will no longer depend on the type of object actually handling the event. Your use case would then change to something like this:

    using namespace std::placeholders;

class Listener
{
public:
    int _i;
    DelegateContainer<int, char> _myDelegate;

    Listener(int i) : _i(i), _myDelegate(std::bind(&Listener::DelegateMethod, this, _1, _2))
    {}

    void DelegateMethod(int i, char c)
    {
        std::cout << ' ' << i * _i << ' ' << c << std::endl;
    }//DelegateMethod
};

    It would have the advantage of allowing other things than non-const non-volatile member functions as event receivers.

  8. You might also look into existing solutions (either to use them or to take inspiration from them), such as Boost.Signals2.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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