HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

C++14 Event System

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
systemeventstackoverflow

Problem

I've created a simplified event system for C++14. The code is commented, so it shouldn't be hard to read. There's also a simple usage scenario below.

It is still a work in progress and uses some not-so-best practices. For example any consumer, that has an access to the event can fire it. Also you will notice, that the subscribers (event handlers) are put into an unordered_map, so they need a unique key, by which we could later determine, whether such a handler already exists. I get this key by 'unionizing' the function object with a uint64_t, multiplying by 10 and then by adding the instance address. I doubt it is a very good idea and platform independent.

Any ideas and critique is welcome.

```
#ifndef __PD_EVENT__
#define __PD_EVENT__

#define PD_EVENT_VER 8

#include
#include
#include
#include

using namespace std::placeholders;

//
// @brief Describes additional parameters for handler binding.
//
enum class EventFlag {
DEFAULT = 0,
ONLY_UNIQUE = 1
};

//
// @version 8
// @brief Defines means for delegate function subscription and calling on demand.
// Maximum allowed number of event arguments is 4.
// @param
//
template
class Event{

//
// @brief Address representation for use as identifier in container mapping.
//
using Address = uint64_t;

//
// @brief Function object with user-defined variadic template arguments.
//
using Handler = std::function;

//
// @brief Type that provides a unique memory-based integral definition for given objects.
// Concept: should be collision-free.
//
using Identifier = uint64_t;

//
// @brief Maximum supported event handler arguments.
//
static constexpr auto _MAX_EVENT_ARGS = 4;
static_assert(sizeof...(Args) event.bind(&MyClass::member, myClassInstance).
//
template
void bind(C (M::member)(), T instance, EventFlag flag = EventFlag::DEFAULT) {
_addToList(_identify(instance, member), std::bind(membe

Solution

Reserved Identifiers

Unless you are very sure of the rules about what combinations of leading underscore are reserved for the implementation I would suggest that you do not use underscore (_) as a prefix. Please see this excellent answer for what identifiers are reserved.

In particular underscore followed by another underscore or a capital letter is reserved for use by the implementation. For example _MAX_EVENT_ARGS is in violation of this rule.
Infinite recursion

You system is susceptible to infinite recursion (and crash by stack overflow). Although any small example I can construct will feel contrived and pathological, when using an event system in a complex UI these kind of problems do crop up.

Consider the following (contrived) example:

class CurrencyConverter{
public:
    Event euroChanged;
    Event dollarChanged;
    int euro;
    int dollar;

    void onEuroChanged(int v){
       dollar = convert2dollar(v);
       dollarChanged(dollar);
    }

    void onDollarChanged(int v){
       euro = convert2euro(v);
       euroChanged(dollar);
    }
}


You could argue that this is invalid use of the system in the same way as void f(){f();} is an improper use of recursion. The difference here is that in an event system this kind of recursive behaviour is not always as apparent as it is here, and it may involve a multiply deep call chain from several different objects.

I believe that you should implement some kind of checking to make sure that recursion into an already executing event handler is caught and reported early. Or possibly just stopped and turned into a no-op. In the above example if the recursion into the current caller just immediately returned the behaviour would be correct.
Dangling pointers

Another problem with the system is that it is susceptible to dangling pointers. Consider the following (again contrived) example:

Event x;
{
    MyClass m;
    x.bind(&MyClass::onX, &m);
}
x.fire(...); // Ka-blamo!


Now of course the fault here is that you forgot to call unbind but explicit managing of bind/unbind takes us back to the age of raw new/delete with all the associated problems. I really believe that you need to RAII-fy this system to avoid this kind of bugs.

One way to do this is to make bind to take std::shared_ptr instead of T* and then change subscribers to store , Handler>. Of course this has the drawback that you cannot bind to anything that isn't a shared pointer and this may be unacceptable.

Another approach is to use something similar to std::lock_guard but instead have bind return a EventGuard like so: EventGuard bind(...) that will unbind when it destructs. This is not foolproof either but it shows a few ideas for how you could approach this.
Collisions in the Identity

The function to calculate an unique identifier is flawed and is riddled with collisions:

inline static Identifier _identify(C _class, M _member) {
    return AddressCast(_class).address * 10 + AddressCast(_member).address;
}


I read the above as: i = a 10 + m. Lets call the lowest address that the system can assign to user heap or stack for a_min, conversely call the largest address a_max. The same goes for m with m_min and m_max. The only way the above expression can be safe is if a_min10 > m_max and a_min10 > a_max. I fired up my debugger and grabed the first heap pointer I could find: a_min a_min10 !> m_max which means that you have collisions in your identity function. This is asking for trouble...
Closing remarks

I think that you can get rid of your template overloads by clever use of a better identification scheme and using variadic template arguments. However I will not go into detail on this due to lack of time.

Code Snippets

class CurrencyConverter{
public:
    Event<int> euroChanged;
    Event<int> dollarChanged;
    int euro;
    int dollar;

    void onEuroChanged(int v){
       dollar = convert2dollar(v);
       dollarChanged(dollar);
    }

    void onDollarChanged(int v){
       euro = convert2euro(v);
       euroChanged(dollar);
    }
}
Event<X> x;
{
    MyClass m;
    x.bind(&MyClass::onX, &m);
}
x.fire(...); // Ka-blamo!
inline static Identifier _identify(C _class, M _member) {
    return AddressCast<C>(_class).address * 10 + AddressCast<M>(_member).address;
}

Context

StackExchange Code Review Q#118628, answer score: 4

Revisions (0)

No revisions yet.