C++ delegate implementation with member functions

I have been implementing a delegate class with C++11 that supports class member functions. I am interested in knowing if there are any potential problems with this implementation. My main concern is the private Key struct of the Delegate class. Can I always rely on the key being correct in all situations? Meaning, can I always identify certain function of certain object with it?

In there I am storing 3 things:

• hash of the member function pointer

• pointer to the object

• std::function wrapping the actual function

The hash is calculated from the function pointer with the getHash() function. I've originally used only that as key, but I run into a problem with inherited classes. I've gotten the same hash for the inherited function with 2 different classes inheriting from same base class. For that reason, I've also stored the pointer to the actual object, using these 2 as key I am able to identify certain entry in the list of stored keys. I'd be really interested to know any potential issues with this implementation. This compiles with VS2013, and I am hoping it will also work with Clang, but I haven't tried it out yet.

```
template
class Delegate {
private:

typedef std::function FunctionType;
struct Key
{
size_t m_funcHash;
void* m_object;
FunctionType m_func;

Key(void* object, size_t funcHash, FunctionType func) : m_object(object), m_funcHash(funcHash), m_func(func)
{}

bool operator==(const Key& other) const {
return other.m_object == m_object && other.m_funcHash == m_funcHash;
}
};

std::vector m_functions;

public:
template
void connect(Class& obj, void (Class::*func) (Params...) )
{

std::function f = func;
FunctionType fun = &obj, f { f(obj, params...); };
size_t hash = getHash(func);
m_functions.push_back(Key( &obj, hash, fun));
}

template
size_t getHash(void (Class::*func) (Params...)) {
const char *ptrptr = static_cast(static_c

Fatal bugs:

A big issue is the fact that you take a copy of the object in the function in this line:

std::function   f = func;

This needs to be Class& at the very least. Otherwise, invocations of handlers would operate on copies of the intended objects. See demonstration here: Live On Coliru with debug information showing copies are being made.

Conceptual issues:

-
you are using a hash for equality. This is a conceptual flaw: hashes can have collisions. Hashes are used to organize large domains into a smaller set of buckets.


this could really be considered a fatal bug, as disconnect might disconnect unrelated handlers here

-
you're wrapping your member-function pointers in a std::function. Twice. This is gonna be very bad for anything performance-sensitive

-
you are abusing std::function for generic calleables. Instead, use perfect storage on user-supplied calleables (why bother whether it's std::mem_fun_ref_t, a bind-expression, a lambda or indeed a function pointer?). This way

• you don't incur the overhead of type erasure and virtual dispatch that comes with std::function, unless your situation requires it

• you don't have to bother with the annoying details that you have to, now

• you don't need to treat member functions differently to begin with (std::bind to the rescue)

• you don't need to rely on implementation defined implementation of pointer-to-member-function

-
you're using implementation defined behaviour to get a hash of a pointer-to-member-function (see previous bullet)

-
your less-than generic solution works for pointer-to-member-function only. This you already knew. But, you may not have realized, it doesn't work for const or volatile qualified member functions.

-
your Delegate class tries to "guess" identity of registered handlers. In fact, the registering party should be responsible for tracking exactly which registration it manages. A cookie-based design would be most suitable here, and certainly takes all the implementation defined behaviour out of the equation (as well as immediately making it clear what happens when the same handler gets registered multiple times). See the "more drastic rewrite" below.

Style, Efficiency

-
Your Key class isn't the Key. It's the collection Entry (Key + Function).

-
Your Key defines operator==. An object that's equatable necessitates linear search to do a lookup. Instead, define a weak total ordering (using operator

-
Also, need to use Key::operator== instead of just comparing m_object and m_funcHash again (encapsulation)

-
Your Key stores the object, but the object is also part of FunctionType implicitely, because you capture it by reference. You should probably remove the redundancy.

-
Your disconnect lookup is ... not very effective:

• linear search? really?

-
no early out? (it's unclear how you really want repeatedly registered callbacks to be handled, but I'd say

del.connect(some_handler);
 del.connect(some_handler);
 del.disconnect(some_handler); // disconnect 1 of the two
 del.disconnect(some_handler); // disconnect the other

would be the principle of least surprise)

-
your i-- invokes unsigned integer wraparound. Now, this is not undefined behaviour, but it's bad style IMO. You could use

for(unsigned int i = 0; i < m_functions.size(); ) {
    auto key = m_functions[i];
    if(key.m_funcHash == hash && key.m_object == &obj) {
        m_functions.erase(m_functions.begin() + i);
    } else
    {
        ++i;
    }
}

instead.

-
the loop crucially depends on m_functions.size() being evaluated each iteration. This is inefficient and error-prone


Guideline: Whenever you see a loop that became... non-trivial and obscure like this, it's a sure sign you need to use an (existing) algorithm

Fixing just these elements: just use a better datastructure, like

struct Key {
    void*         m_object;
    size_t        m_funcHash;
    // no ctor: let just just be an aggregate

    bool operator==(const Key& other) const { return key() == other.key(); }
    bool operator key() const { return std::tie(m_object, m_funcHash); }
};

std::multimap m_functions;

The whole disconnect function becomes trivial:

template 
void disconnect(Class& obj, void (Class::*func)(Params...)) {
    m_functions.erase(Key { &obj, getHash(func) });
}

Fixed version(s)

Here's a version that uses the multimap approach (fixing just the style/efficiency issues) Live On Coliru

A more drastic rewrite

would look like this: Live On Coliru

This version alters the design so most (if not all) the problems are sidestepped. It uses boost's stable_vector to do a cookie-based design. This moves the burden to track handler identities on the registering parties (where it belongs after all).

``
#include
#include
#include

template
class Delegate {
private:
typedef std::function Handler;
typedef boost::container::stable_vector Vector;
Vector m_handlers;

StackExchange Code Review Q#36251, answer score: 3