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 &lt;typename...Params&gt;
class Delegate {
private:    

typedef std::function &lt; void(Params...)&gt; 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&amp; other) const {
        return other.m_object == m_object &amp;&amp; other.m_funcHash == m_funcHash;
    }
};

std::vector &lt;Key&gt; m_functions;

public:
template &lt;typename Class&gt;
void connect(Class&amp; obj, void (Class::*func) (Params...) ) 
{

    std::function &lt; void (Class, Params...) &gt;  f = func;
    FunctionType fun = [&amp;obj, f](Params... params) { f(obj, params...); };
    size_t hash = getHash(func);
    m_functions.push_back(Key( &amp;obj, hash, fun));
}

template &lt;typename Class&gt;
size_t getHash(void (Class::*func) (Params...)) {
    const char *ptrptr = static_cast&lt;const char*&gt;(static_cast&lt;const void*&gt;(&amp;func));
    int size = sizeof (func);
    std::string str_rep(ptrptr, size);
    std::hash&lt;std::string&gt; strHasher;
    return strHasher(str_rep);
}

template &lt;typename Class&gt;
void disconnect( Class&amp; obj, void (Class::*func) (Params...)  ) {
    size_t hash = getHash(func);

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

template &lt;typename ... Args&gt;
void operator() (Args...args)
{       
    for (auto&amp; key : m_functions) {
        key.m_func (args...);
    }
}
};


class BaseClass {

public:
    virtual void print(const std::string&amp; str) = 0;
};

class A : public BaseClass {
public:
    void print(const std::string&amp; str) {
        std::cout &lt;&lt; " Class A : Print [" &lt;&lt; str &lt;&lt; "]\n";
    }

};

class B : public BaseClass {
public:
    void print(const std::string&amp; str) {
        std::cout &lt;&lt; " Class B : Print [" &lt;&lt; str &lt;&lt; "]\n";
    }
};



int _tmain(int argc, _TCHAR* argv[])
{

    A a;
    B b;

    Delegate &lt;const std::string&amp;&gt; delegate;

    delegate.connect(a, &amp;A::print);
    delegate.connect(b, &amp;B::print);

    delegate("hello");
    delegate("world");

    delegate.disconnect(a, &amp;A::print);
    delegate("bye world");

    delegate.disconnect(b, &amp;B::print);  
    delegate("nobody there.");  // should not print anything

    std::cin.ignore();

    return 0;
}
Fatal bugs:

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

    std::function &lt; void (Class, Params...) &gt;  f = func;


This needs to be Class&amp; 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&lt;) so you can use tree-like data-structures. See below for a sample that uses std::tie() to quickly implement both relational operators (the first version).
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 &lt; m_functions.size(); ) {
    auto key = m_functions[i];
    if(key.m_funcHash == hash &amp;&amp; key.m_object == &amp;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&amp; other) const { return key() == other.key(); }
    bool operator&lt; (const Key&amp; other) const { return key() &lt;  other.key(); }
  private:
    // trick to make it trivial to consistently implement relational operators:
    std::key&lt;void* const&amp;, size_t const&amp;&gt; key() const { return std::tie(m_object, m_funcHash); }
};

std::multimap&lt;Key, FunctionType&gt; m_functions;


The whole disconnect function becomes trivial:

template &lt;typename Class&gt;
void disconnect(Class&amp; obj, void (Class::*func)(Params...)) {
    m_functions.erase(Key { &amp;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 &lt;functional&gt;
#include &lt;iostream&gt;
#include &lt;boost/container/stable_vector.hpp&gt;

template &lt;typename... Params&gt;
class Delegate {
private:
    typedef std::function&lt;void(Params...)&gt; Handler;
    typedef boost::container::stable_vector&lt;Handler&gt; Vector;
    Vector m_handlers;
public:
    typedef typename Vector::const_iterator cookie;

    cookie connect(Handler&amp;&amp; func) {
        m_handlers.push_back(std::move(func));
        return m_handlers.begin() + m_handlers.size() - 1;
    }

    template &lt;typename... BindArgs, typename Sfinae = typename std::enable_if&lt;(sizeof...(BindArgs)&gt;1), void&gt;::type&gt;
    cookie connect(BindArgs&amp;&amp;... args) {
        return connect(Handler(std::bind(std::forward&lt;BindArgs&gt;(args)...)));
    }

    void disconnect(cookie which) {
        m_handlers.erase(which);
    }

    template &lt;typename ... Args&gt; void operator()(Args...args) {
        for(auto const&amp; handler : m_handlers) 
            handler(args...);
    }
};

//////////////////////////////////
// demonstration
#define CV volatile const
struct BaseClass {
    virtual void print(const std::string&amp; str) CV = 0;
};

struct A : BaseClass {
    A() {}
    void print(const std::string&amp; str) CV { std::cout &lt;&lt; " Class A : Print [" &lt;&lt; str &lt;&lt; "]\n"; }
};

struct B : BaseClass {
    B() {}
    void print(const std::string&amp; str) CV { std::cout &lt;&lt; " Class B : Print [" &lt;&lt; str &lt;&lt; "]\n"; }
};

using namespace std::placeholders;

int main() {
    Delegate &lt;const std::string&amp;&gt; delegate;
    A CV a;
    B CV b;
    auto acookie = delegate.connect(&amp;A::print, std::ref(a), _1);
    auto bcookie = delegate.connect(&amp;B::print, std::ref(b), _1);

    delegate("hello");
    delegate("world");

    delegate.disconnect(acookie);
    delegate("bye world");

    delegate.disconnect(bcookie);
    delegate("nobody there.");  // should not print anything
}