Skip to main content
Code Review

Return to Revisions

10 of 11
added 406 characters in body

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_asserts 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);
 }

Here is a Demo

You can avoid that adding another static_assert:

static_assert(!std::is_polymorphic<TFunctor>(),"Functor object must not be polymorphic.");

Demo

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?

default

AltStyle によって変換されたページ (->オリジナル) /