C++ delegate implementation with member functions

Question

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:

1. hash of the member function pointer
2. pointer to the object
3. 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 <typename...Params>
class Delegate {
private: 
typedef std::function < void(Params...)> 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 <Key> m_functions;
public:
template <typename Class>
void connect(Class& obj, void (Class::*func) (Params...) ) 
{
 std::function < void (Class, Params...) > f = func;
 FunctionType fun = [&obj, f](Params... params) { f(obj, params...); };
 size_t hash = getHash(func);
 m_functions.push_back(Key( &obj, hash, fun));
}
template <typename Class>
size_t getHash(void (Class::*func) (Params...)) {
 const char *ptrptr = static_cast<const char*>(static_cast<const void*>(&func));
 int size = sizeof (func);
 std::string str_rep(ptrptr, size);
 std::hash<std::string> strHasher;
 return strHasher(str_rep);
}
template <typename Class>
void disconnect( Class& obj, void (Class::*func) (Params...) ) {
 size_t hash = getHash(func);
 for (unsigned int i = 0; i < m_functions.size(); ++i) {
 auto key = m_functions[i];
 if (key.m_funcHash == hash && key.m_object == &obj) {
 m_functions.erase(m_functions.begin() + i);
 --i;
 }
 }
}
template <typename ... Args>
void operator() (Args...args)
{ 
 for (auto& key : m_functions) {
 key.m_func (args...);
 }
}
};
class BaseClass {
public:
 virtual void print(const std::string& str) = 0;
};
class A : public BaseClass {
public:
 void print(const std::string& str) {
 std::cout << " Class A : Print [" << str << "]\n";
 }
};
class B : public BaseClass {
public:
 void print(const std::string& str) {
 std::cout << " Class B : Print [" << str << "]\n";
 }
};
int _tmain(int argc, _TCHAR* argv[])
{
 A a;
 B b;
 Delegate <const std::string&> delegate;
 delegate.connect(a, &A::print);
 delegate.connect(b, &B::print);
 delegate("hello");
 delegate("world");
 delegate.disconnect(a, &A::print);
 delegate("bye world");
 delegate.disconnect(b, &B::print); 
 delegate("nobody there."); // should not print anything
 std::cin.ignore();
 return 0;
}

Answer 1

Fatal bugs:

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

 std::function < void (Class, Params...) > 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<) 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 < 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< (const Key& other) const { return key() < other.key(); }
 private:
 // trick to make it trivial to consistently implement relational operators:
 std::key<void* const&, size_t const&> key() const { return std::tie(m_object, m_funcHash); }
};
std::multimap<Key, FunctionType> m_functions;

The whole disconnect function becomes trivial:

template <typename Class>
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 <functional>
#include <iostream>
#include <boost/container/stable_vector.hpp>
template <typename... Params>
class Delegate {
private:
 typedef std::function<void(Params...)> Handler;
 typedef boost::container::stable_vector<Handler> Vector;
 Vector m_handlers;
public:
 typedef typename Vector::const_iterator cookie;
 cookie connect(Handler&& func) {
 m_handlers.push_back(std::move(func));
 return m_handlers.begin() + m_handlers.size() - 1;
 }
 template <typename... BindArgs, typename Sfinae = typename std::enable_if<(sizeof...(BindArgs)>1), void>::type>
 cookie connect(BindArgs&&... args) {
 return connect(Handler(std::bind(std::forward<BindArgs>(args)...)));
 }
 void disconnect(cookie which) {
 m_handlers.erase(which);
 }
 template <typename ... Args> void operator()(Args...args) {
 for(auto const& handler : m_handlers) 
 handler(args...);
 }
};
//////////////////////////////////
// demonstration
#define CV volatile const
struct BaseClass {
 virtual void print(const std::string& str) CV = 0;
};
struct A : BaseClass {
 A() {}
 void print(const std::string& str) CV { std::cout << " Class A : Print [" << str << "]\n"; }
};
struct B : BaseClass {
 B() {}
 void print(const std::string& str) CV { std::cout << " Class B : Print [" << str << "]\n"; }
};
using namespace std::placeholders;
int main() {
 Delegate <const std::string&> delegate;
 A CV a;
 B CV b;
 auto acookie = delegate.connect(&A::print, std::ref(a), _1);
 auto bcookie = delegate.connect(&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
}

• sehe: Oh, and consider exception safety of the handlers. And error handling in case of invalid cookies.
• KJS: Hey, thanks for a great answer! Really good points on how to improve. The linear search at disconnect was planned actually, as function could have been registered more than once.
• sehe: Yeah, linear lookup is probably fine for usual numbers of registered handlers. Disconnecting the same more than once is handled (much cleaner) using the multimap approach: coliru.stacked-crooked.com/a/658e6d086eb2f859. Bear in mind you still need to prevent the problem of abusing the hash for identity
• sehe: @KJS If you insist on a cookie-less/token-less design you can never support lambdas or bind expressions, I think. In that case you still need to do work to allow for const-volatile cases (use sfinae to avoid overload swamp): cv_qualified_fix.cpp . And as that Coliru shows, you will still want to check properly for different ptmf with the same hash. You might add a typeindex to disambiguate: disambiguating_fix.cpp
• KJS: I'm willing to try solutions that can be implemented with standard library (i'm not currently using boost at all). About the hashing, I was thinking about just checking if this object already has a function registered with that hash, and assert if so. Overall this review contains a lot of new information for me, I'll need to dig in to the details and learn them :)