Storing (and retrieving) functions by indexing them by address

Question

I want to implement a "service" of callbacks, where subscribers register or unregister a callback. I was thinking about indexing the callbacks by the function address, so that to unregister one would simply have to pass the same function.

So, disregarding threading issues, the simplest form of my implementation would be

#include <functional>
#include <map>
#include <cstdint>

std::map<std::uintptr_t, std::function<void()>> callbacks;

///https://stackoverflow.com/a/18039824/2436175
namespace {
std::uintptr_t get_address(std::function<void()> f) {
    typedef void(function_type)();
    function_type** function_pointer = f.template target<function_type*>();
    return static_cast<std::uintptr_t>(*function_pointer);
}
}// namespace

void subscribe(std::function<void()>&& callback) {
    callbacks.insert(std::make_pair(get_address(callback), std::move(callback)));
}

void unsubscribe(const std::function<void()>& callback) {
    callbacks.erase(get_address(callback));
}

void execute() {
    for (const auto& callback : callbacks) {
       callback.second.operator()();
    }
}

On the subscriber side, we would have things like:

void test() {
}
    
subscribe(test);
unsubscribe(test);

I tested and it would seem to work. See code sample.

Assuming I will only use plain functions like test in this example (no lambdas, no bound methods...), is this a solid mechanism?

Answer 1

Don't use std::function<> if you are only going to allow plain functions

I would avoid using std::function<> if your code doesn't handle something other than a plain function being bound to it. As Deduplicator mentioned, why not use regular function pointer types to store the callbacks?

Prefer using over typedef

Especially for function pointers, the typedef syntax is a bit hard to read. Prefer using, like so:

using function_type = void();

Use reinterpret_cast<> instead of a C-style cast

Avoid C style casts in general, and use the best fitting C++-style cast instead. In this case:

return reinterpret_cast<uintptr_t>(*function_pointer);

Naming things

You named the result of the call to target() function_pointer, but it's not a function pointer; it is a pointer to a function pointer. I would rename it to target_pointer to avoid any confusion.

Don't use an r-value reference in subscribe()

Your use of an r-value reference and subsequent use of std::move() is looking very dodgy. It might be fine if you only pass regular functions to it, but then there is no point of using move semantics, as std::function will not do any allocation then. Furthermore, doing something like:

foo(bar, std::move(bar));

Will resulted in undefined behaviour, as the order of evaluation of the function parameters is undefined. So the compiler is allowed to do the move first, then pass the moved-from bar as the first argument. You could call get_address() first and store the results in a temporary variable, but I would avoid the whole issue altogether and pass the std::function by value.

Use emplace()

You can avoid having to call std::make_pair by using emplace():

callbacks.emplace(get_address(callback), callback);

Consider using structured bindings

Since you tagged the question C++17, consider using structured bindings to avoid having to use first and second if you iterate over a map:

for (const auto&[address, callback]: callbacks) {
    callback();
}

Consider using a std::unordered_set

Since you don't care about the ordering of the callbacks, you can use a std::unordered_map instead. This will allow for faster insertion and removal into callbacks.

However, storing both the address and the function in a map is redundant; you can derive the address from the function as you are already doing. So instead you could use a std::unordered_set. It is a bit more cumbersome to make a std::unordered_set of a std::function, but if you would only store regular function pointers, you can do:

std::unordered_set<void(*)()> callbacks;