C++ Generic Callback class with removable listeners by unique id

Question

I'm quite new to the STL.

Does this make sense? Is there a better way of removing the listeners instead of using shared_ptr while keeping the code short and simple? Is there something in the STL to replace this?

Class:

template<class ... Arguments>
class Callback
{
    using Function = std::function<void(Arguments ...)>;
    using FunctionPtr = std::shared_ptr<Function>;
    using Functions = std::vector<FunctionPtr>;

public:
    FunctionPtr addListener(Function f) {
        FunctionPtr fp = std::make_shared<Function>(f);
        _functions.push_back(fp);
        return fp;
    }

    void removeListener(FunctionPtr fp){
        _functions.erase(std::find(_functions.begin(), _functions.end(), fp));
    }

    void operator()(Arguments ... args) const{
        for (auto & f : _functions)
            f.get()->operator()( args ... );
    }

private:
    Functions _functions;
};

Use case:

int main()
{
    Callback<int> callback;

    callback.addListener([](int i){
        std::cout << "Listener 1 -> Input is " << i <<  std::endl;
    });

    auto id = callback.addListener([](int i){
        std::cout << "Listener 2 -> Input is " << i <<  std::endl;
    });

    callback.addListener([](int i){
        std::cout << "Listener 3 -> Input is " << i <<  std::endl;
    });

    callback.removeListener(id);

    callback(71);
}

Output is:

Listener 1 -> Input is 71
Listener 3 -> Input is 71

Addendum One limitation of this approach is that you can't remove or add listeners inside the callback functions since it would break the iteration. What I found is that this limitation is especially annoying when you want something to only trigger once and then be removed. The easiest solution I found is to simply have a separate list of callbacks that are called once and then removed automatically after the iteration. Another easy, more generic solution is to copy the listeners container before triggering the callbacks with the obvious performance cost.

Answer 1

I think it's quite OK like that. Of course that depends on the use case and your goal.

One thing I would definitely change though is to return only a std::weak_ptr from addListener.

Here a few alternative approaches: replace the std::vector with

• a std::set or std::unordered_set and keep the std::weak_ptr as the "ID", or
• a std::list and use the iterator as "ID", or
• a std::map or std::unordered_map and make up your own ID, maybe just as simple as a static integer counter.

Those all make the removal faster (logarithmic or constant instead of linear). The addition is already and would stay constant (at least amortized).

Also if you want the returned "ID" to be a little more "ID-like" and don't want to use a map you could wrap the std::weak_ptr or the std::list::iterator into an Id class. And if you then want the single member of the Id class to not be accessable by the user make it private and Callback a friend.

---

Further non question related remarks:

Adding the missing header would have been a good polish for the otherwise good and complete question to make it truly copy&paste&runnable.

You could add a few const/references here and there:

• addListener argument can be const&&
• removeListener argument can be const&
• operator() arguments can be const&
• the f in the for loop could also be const

although it will not always help that much.

Answer 2

I think this makes sense in terms of STL use. Storing a std::shared_ptr<std::function> is not usually necessary since std::function may own heap allocations internally, but since you return the shared_ptr from addListener it could be passed around. If you never use the returned shared_ptr, you could just store a std::vector<std::function> and return a handle for removal (maybe the handle could contain the index of the function in the vector?).

I can't say whether a vector of functions makes sense for your use case. If you can avoid callbacks and call functions directly, that's definitely preferable -- makes optimization much easier for the compiler.

---

Other stuff:

class Callback
{
    using Function = std::function<void(Arguments ...)>;

These typedefs are nice, but could be better. They should be public since they are used in the public API. And perhaps the names could be clearer (e.g. VecPtrFunc instead of Functions)?

_functions.erase(std::find(_functions.begin(), _functions.end(), fp));

This seems OK, but if you call this a lot or if functions is big, this will be slow. You might prefer to use a data structure with sub linear lookup/delete like std::map or std::unordered_map. Or you could use the index-handle idea above and remove from a vector more efficiently.

void operator()(Arguments ... args) const

If you were just calling one callee, this would be Arguments&& and you'd std::forward the args. Since you use the args... in a loop, I guess this is fine. Maybe Arguments const& is better in case folks pass in big types and don't want to copy?

Answer 3

I used an unordered_map<string, FunctionPtr> because you can use f.target_type().name() as the ID. That way you can overload removeListener to either take the Function again or a string.

template <class... Arguments>
class Callback {
    using CallbackFunc = std::function<void(Arguments...)>;
    using CallbackFuncPtr = std::shared_ptr<CallbackFunc>;

   public:
    ~Callback() {
        _functions.clear();
    }

    std::string addListener(const CallbackFunc& f) {
        const std::string func = f.target_type().name();
        CallbackFuncPtr fp = std::make_shared<CallbackFunc>(f);
        _functions[func] = fp;
        return func;
    }

    bool removeListener(const CallbackFunc& f) {
        const std::string func = f.target_type().name();
        return removeListener(func);
    }

    bool removeListener(const std::string& func) {
        auto l = _functions.find(func);
        if (l != _functions.end()) {
            _functions.erase(l);
            return true;
        } else {
            std::cerr << "Callback::removeListener(): listener not found" << std::endl;
        }
        return false;
    }

    void operator()(Arguments... args) const {
        for (const auto& [key, value] : _functions) {
            value.get()->operator()(args...);
        }
    }

   private:
    std::unordered_map<std::string, CallbackFuncPtr> _functions;
};