trjhtr

I tried to implement a simple signal&slots mechanism for some DSP software that runs on embedded Linux. I would like if somebody can review this code and give me some guidelines. Any review and advice is welcome.
Main idea was to have a signal as a data member inside any of my classes that receive data from DSP and emit that data to multiple of dsp pipeline processing classes.

Classes and their description

• 
  slot_key - This is class responsible for assigning ID to any newly created instance of class slot. Implementation of this class is something I really don't like. This class is used as a base class of class slot. slot_key::slot_id is used in class signal as a key to store said slot and to be able to disconnect slot from signal.
  (although in my case connected slots do not disconnect during life time of the application).

Code:

using slot_id_t = uint64_t;
struct slot_key 
 slot_id_t slot_id;
 slot_key() : slot_id(0) 
 slot_key(const slot_key&) : slot_id(0) 

protected:
 static uint64_t _slots_id;
 slot_key(uint64_t) : slot_id(++_slots_id) 
;

uint64_t slot_key::_slots_id = 0;

• 
  slot - This is class that holds any callable object client programmer wants to connect with wanted signal. slot is able to hold any callable object (functor, lambda, class instance + function member of that class or global function), only limitation for underlying object is to be callable with argumets signal emits.

Code:

template <class... Args>
struct slot : slot_key 
 slot() 

 template<class T, class R>
 slot(T& t, R r) :
 slot_key(0),
 _pcallback (new class_holder<T>(t, r))
 

 template<class T>
 slot(T& t) :
 slot_key(0),
 _pcallback (new functor_holder<T>(t))
 

 slot(void(fp)(Args...)) :
 slot_key(0),
 _pcallback (new function_holder<void(Args...)>(fp))
 

 void operator()(Args... args) 
 (*_pcallback)(args...);
 

private:
 template <class... A>
 struct call_interface 
 virtual void operator ()(A... args) = 0;
 ;

 template <class owner>
 struct class_holder : call_interface<Args...> 
 using method_type = void(owner::*)(Args...);
 class_holder(owner &o, method_type method) :
 _method(method), _owner(o)
 

 void operator ()(Args... args) 
 (_owner.*_method)(args...);
 

 method_type _method;
 owner& _owner;
 ;

 template <class owner>
 struct functor_holder : call_interface<Args...> 
 functor_holder(owner &o) : _owner(o)
 

 void operator ()(Args... args) 
 _owner(args...);
 

 owner& _owner;
 ;

 template <class fn>
 struct function_holder : call_interface<Args...> 
 function_holder(fn* func) : _func(func)
 

 void operator ()(Args... args) 
 (*_func)(args...);
 

 fn* _func;
 ;

 std::unique_ptr<call_interface<Args...>> _pcallback;
;

• 
  signal - This is class used for emitting data to slots. It's implementation is simple. It provides interface for connecting any callable element and returning slot_id that is used if disconnect from signal is wanted.

Code:

template <class... Args>
struct signal 

 signal() = default;
 signal(signal&&) = default;

 // Copied signal doesnt have connected slots from original
 signal(const signal&) : 
 slots(std::map<slot_id_t, slot<Args...>>()) 

 void emit(Args... args) 
 for (auto &s : slots)
 s.second(args...);
 

 template<class T> // for classes and class function member
 slot_id_t connect(T& t, void(T::*fp)(Args...)) 
 return add_slot(slot<Args...>(t, fp));
 

 template<class T> // for functors
 slot_id_t connect(T& t) 
 // TODO: in C++17 is_invocable can be used
 static_assert(is_callable<T>::value, "Parameter not invokable. 
 Pass method you want to use as slot"); 
 return add_slot(slot<Args...>(t));
 

 slot_id_t connect(void(fp)(Args...)) // for global functions
 return add_slot(slot<Args...>(fp));
 

 void disconnect(slot_id_t slot) 
 slots.erase(slot);
 

private:
 slot_id_t add_slot(slot<Args...>&& t) 
 slots[t.slot_id] = std::move(t);
 return t.slot_id;
 

 std::map<slot_id_t, slot<Args...>> slots;
;

File with all classes and usage example (it should be able to compile as is)

main.cpp:

#include <iostream>
#include <vector>
#include <map>
#include <functional>
#include <memory>
#include <type_traits>


template<typename C, typename = void>
struct is_callable : std::false_type ;

template<typename C>
struct is_callable<C, std::void_t<decltype(&C::operator())>> : std::true_type ;


template<class ...Args>
struct signal;

using slot_id_t = uint64_t;
struct slot_key 
 slot_id_t slot_id;
 slot_key() : slot_id(0) 
 slot_key(const slot_key&) : slot_id(0) 

protected:
 static uint64_t _slots_id;
 slot_key(uint64_t) : slot_id(++_slots_id) 
;

uint64_t slot_key::_slots_id = 0;

template <class... Args>
struct slot : slot_key 
 slot() 

 template<class T, class R>
 slot(T& t, R r) :
 slot_key(0),
 _pcallback (new class_holder<T>(t, r))
 

 template<class T>
 slot(T& t) :
 slot_key(0),
 _pcallback (new functor_holder<T>(t))
 

 slot(void(fp)(Args...)) :
 slot_key(0),
 _pcallback (new function_holder<void(Args...)>(fp))
 

 void operator()(Args... args) 
 (*_pcallback)(args...);
 

private:
 template <class... A>
 struct call_interface 
 virtual void operator ()(A... args) = 0;
 ;

 template <class owner>
 struct class_holder : call_interface<Args...> 
 using method_type = void(owner::*)(Args...);
 class_holder(owner &o, method_type method) : _method(method), _owner(o)
 

 void operator ()(Args... args) 
 (_owner.*_method)(args...);
 

 method_type _method;
 owner& _owner;
 ;

 template <class owner>
 struct functor_holder : call_interface<Args...> 
 functor_holder(owner &o) : _owner(o)
 

 void operator ()(Args... args) 
 _owner(args...);
 

 owner& _owner;
 ;

 template <class fn>
 struct function_holder : call_interface<Args...> 
 function_holder(fn* func) : _func(func)
 

 void operator ()(Args... args) 
 (*_func)(args...);
 

 fn* _func;
 ;

 std::unique_ptr<call_interface<Args...>> _pcallback;
;

template <class... Args>
struct signal 

 signal() = default;
 signal(signal&&) = default;
 signal(const signal&) : slots(std::map<slot_id_t, slot<Args...>>()) // Copied signal doesnt have connected slots from original

 void emit(Args... args) 
 for (auto &s : slots)
 s.second(args...);
 

 template<class T> // for classes and class function member
 slot_id_t connect(T& t, void(T::*fp)(Args...)) 
 return add_slot(slot<Args...>(t, fp));
 

 template<class T> // for functors
 slot_id_t connect(T& t) 
 static_assert(is_callable<T>::value, "Parameter not invokable. Pass method you want to use as slot"); // TODO: in C++17 is_invocable can be used
 return add_slot(slot<Args...>(t));
 

 slot_id_t connect(void(fp)(Args...)) // for global functions
 return add_slot(slot<Args...>(fp));
 

 void disconnect(slot_id_t slot) 
 slots.erase(slot);
 

private:
 slot_id_t add_slot(slot<Args...>&& t) 
 slots[t.slot_id] = std::move(t);
 return t.slot_id;
 

 std::map<slot_id_t, slot<Args...>> slots;
;

struct signal_provider 
 signal<int, int> sig;
;

struct slot_1 
 void on_signal(int a, int b) 
 std::cout << __PRETTY_FUNCTION__ << " " << a << ", " << b << std::endl;
 
;

struct slot_2 
 void on_signal2(int a, int b) 
 std::cout << __PRETTY_FUNCTION__ << " " << a << ", " << b << std::endl;
 
;

auto lambda = (int a, int b)
 std::cout << __PRETTY_FUNCTION__ << " " << a << ", " << b << std::endl;
;

void global_method(int a, int b) 
 std::cout << __PRETTY_FUNCTION__ << " " << a << ", " << b << std::endl;


int main() 
 slot_1 slot1;
 slot_2 slot2;
 signal_provider signal;
 auto lambda_w_capture_list = [&](int a, int b) std::cout << __PRETTY_FUNCTION__ << " " << a << ", " << b << std::endl; ;

 auto sl_id = signal.sig.connect(slot2, &slot_2::on_signal2);
 signal.sig.connect(slot1, &slot_1::on_signal);
 signal.sig.connect(lambda);
 signal.sig.connect(global_method);
 signal.sig.connect(lambda_w_capture_list);
 signal.sig.emit(5, 6);

 signal.sig.disconnect(sl_id);
 signal.sig.emit(8, 8);

 return 0;

[code] uint64_t

It should be std::uint64_t, not uint64_t for key indexes (the latter is the C version). Or better: use std::size_t, which is the standard indexing type.

[code] passing arguments

At the moment, all the parameter pack arguments to the function calls are being passed by value. This should instead use perfect forwarding to prevent unnecessary copies (and allow passing non-copyable objects by reference or move):

void operator()(Args&&... args) 
 (*_pcallback)(std::forward<Args>(args)...);


...

void emit(Args&&... args) 
 for (auto &s : slots)
 s.second(std::forward<Args>(args)...);

[code / design] signal copy / move

There doesn't seem to be any point in allowing copying a signal if it doesn't have the connected slots from the original, and this is very surprising behaviour for the user. Either disallow copying, or copy the slots completely.

Copy / move assignment operators should be supplied to match the copy / move constructors.

[design] slot_key

The slot_id inside the slot_key class is duplicated in the map structure. Also, the slot class itself doesn't seem to be used or usefully accessible outside of the signal. I'd recommend either:

1. removing the slot_id member from the slot class, and removing the slot_key base class. Or...


2. changing the slots data structure to a std::set, and providing the relevant comparison operator for the slot class.

If you do choose to keep the slot_id member, it would probably be better off in the slot class, rather than a base class (prefer composition to inheritance), and the key generation part might be better off as a key_generator object, existing in the signal.

[design] slot

All the functionality of the slot class can be handled by std::function, which makes the class redundant:

#include <iostream>
#include <map>
#include <functional>
#include <cassert>

using slot_id_t = std::uint64_t;

struct slot_key_generator 
 slot_id_t _slots_id = 0;

 slot_id_t get_next() 
 return _slots_id++;
 
;

template <class... Args>
struct signal 

 using function_t = std::function<void(Args...)>;

 signal() = default;
 signal(signal&&) = default;
 signal(const signal&) = default;

 signal& operator=(signal&&) = default;
 signal& operator=(signal const&) = default;

 void emit(Args&&... args) 
 for (auto &s : _slots)
 s.second(std::forward<Args>(args)...);
 

 slot_id_t connect(function_t function) 
 assert(function); // no empty functions!
 return add_slot(std::move(function));
 

 void disconnect(slot_id_t slot) 
 _slots.erase(slot);
 

private:

 slot_id_t add_slot(function_t&& t) 
 auto key = _generator.get_next();
 _slots.emplace(key, std::move(t));
 return key;
 

 slot_key_generator _generator;
 std::map<slot_id_t, function_t> _slots;
;

The user code is the same, except for functors, which can use std::bind (or a lambda if it's easier):

using namespace std::placeholders;
auto sl_id = signal.sig.connect(std::bind(&slot_2::on_signal2, &slot2, _1, _2));
signal.sig.connect(std::bind(&slot_1::on_signal, &slot1, _1, _2));