A few thoughts about your code:
1. Use of reinterpret_cast
// Create a simple function that calls the functor
this->func_ = [](void *user, TArgs ... args) -> void {
TFunctor *functorPtr = reinterpret_cast<TFunctor*>(user);
(*functorPtr)(args...);
};
This always raises a red flag for me. I understand that the user
parameter is used to pass the actual functor instance, and it's a common technique to interface with C-style callback API's.
May be it deserves more commenting why exactly it will be used here, and what exactly user
is expected to carry.
2. Using a fixed size for the functorData_
member
This makes your class less flexible for generic usage.
You could use an additional template parameter to specify the size
template <size_t MaxFunctorSize, typename ... TArgs>
class Callback final {
// ...
uint8_t functorData_[MaxFunctorSize];
};
and use that with the static_assert
s etc.
At least the callback holder could decide then, how many stack storage should me reserved as maximum.
The downside is, that the maximum size cannot be defaulted to a standard value because of the variadic parameter pack must appear as the last parameter.
3. Wasting memory for sake of making client code easier to write
I'm not a 100% sure that wasting (stack) space is worth that (especially when dealing with a small MCU at the bare metal level).
May be the design could be improved, and less memory intrusive with bit a more complicated SFINAE based class model to handle the different cases
- Simple C-Style function pointer
- Functor object / Lambda expression
4. Potential slicing problems with functor instances
I smell potential slicing problems with virtual polymorphic functor class hierarchies.
The functor interface using functorData_
may fail to work properly with virtual inheritance:
struct IFunctor {
virtual void operator()(const uint8_t*, uint32_t) = 0;
virtual ~IFunctor() = default;
};
struct AFunctor : IFunctor {
virtual void operator()(const uint8_t*, uint32_t) {
// Do something
}
};
void doSomething() {
SerialPort port;
AFunctor aFunc;
IFunctor* iFunc = &aFunc;
port.setReceiveDoneCallback(*iFunc);
}
You can avoid that adding another static_assert
:
static_assert(!std::is_polymorphic<TFunctor>(),"Functor object must not be polymorphic.");
Note that this is related to my points 1. and 2.
5. The use of inline
is superfluous inside the class declaration body
As mentioned above, you don't even need to use inline
inside of the class body declaration. That's merely a hint for the compiler, and will be done (or overridden) regarding the chosen optimization strategy anyways.
The only case you must use the inline
keyword is, if you're defining global functions in a multiply included header file. Otherwise you are going to violate the One Definition Rule principle.
Regarding GCC's behavior, it's documented here (emphasis mine):
GCC does not inline any functions when not optimizing unless you specify the ‘always_inline’ attribute for the function, like this:
/* Prototype. */ inline void foo (const char) __attribute__((always_inline));
Otherwise, it depends on optimization settings.
6. The use of final
is superfluous
Since your class doesn't expose any virtual
functions that could be potentially overridden by inheriting classes, you simply can omit it.
See also: In C++, when should I use final in virtual method declaration?
- 5.2k
- 4
- 23
- 32