5
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ If you are the only person with a pointer to an object why do you need a "shared pointer" just use "unique pointer" it would be more efficient. Why dynamically create a pointer when you can simply store the "Function" in the vector? \$\endgroup\$ Apr 27, 2021 at 22:25
  • \$\begingroup\$ The shared:ptr is there just to provide an unique id to the std::function although as suggested in the answer below it is probably better to return a weak:ptr. I'm not sure if an iterator would also work because I think removing an object from the collection will invalidate it. \$\endgroup\$
    – fede.dev
    Apr 28, 2021 at 22:08
  • 1
    \$\begingroup\$ You could use a container were removing elements does not invalidate existing iterators. But I don't like the idea of iterators either as it gives the user of your class an opportunity to abuse the system (they can call the stored function via the iterator). So I would do some extra work to make the iterator opaque. Or keep track via some index. \$\endgroup\$ Apr 29, 2021 at 15:16

3 Answers 3

2
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Btw I got the perception that you just want to return an ID from your example use case. If you of course want to actually use the inserted and returned function (pointer) then my answer is a whole lot of gibberish of course. \$\endgroup\$ Apr 18, 2021 at 18:08
  • \$\begingroup\$ Yes, the shared_ptr is intended to be used just as an ID so your answer is spot on. I could have made it clearer in the question. \$\endgroup\$
    – fede.dev
    Apr 19, 2021 at 8:34
3
\$\begingroup\$

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?

\$\endgroup\$
2
  • \$\begingroup\$ Keep in mind that if the handle is the index in the vector than some handles might get invalidated on removal of another function. \$\endgroup\$ Apr 18, 2021 at 10:49
  • \$\begingroup\$ that's a good point \$\endgroup\$ Apr 18, 2021 at 16:21
1
\$\begingroup\$

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;
};
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.