Event handler using variadic templates

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

namespace Utilities
{
template
class Event;

///////////////////////////////////////////////////////////////////////////////////////////
// DelegateContainer
///////////////////////////////////////////////////////////////////////////////////////////
template
class DelegateContainer
{
friend class Event;

protected:
std::list*> _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;
};

///////////////////////////////////////////////////////////////////////////////

In no particular order:

-
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(d)
  , _t(d._t)
  , _callback(d._callback)
{}

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

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

Delegate& operator= (Delegate&& d)
{
  DelegateContainer::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.

-
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)...);
    return true;
  }
  return false;
}

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

-
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 
DelegateContainer::~DelegateContainer()
{
  while (!_subscriptions.empty())
    *_subscriptions.front() -= *this;
}

-
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.

-
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.

-
I understand the usage code i

StackExchange Code Review Q#33037, answer score: 5