OpenCyphal-Garage / libcyphal

Portable reference implementation of the Cyphal protocol stack in C++ for embedded systems and Linux.
http://opencyphal.org
MIT License
300 stars 501 forks source link

Executor doesn't implement cetl::rtti #398

Open thirtytwobits opened 1 week ago

thirtytwobits commented 1 week ago

For example: https://github.com/OpenCyphal-Garage/libcyphal/blob/main/include/libcyphal/executor.hpp#L108 We have the same methods defined but they are not virtual and the cetl::rtti interface is not used.

serges147 commented 1 week ago

The line you mention is not really about IExecutor but its Callback::Interface. But you are right - I need review all places where rtti is implemented. The reason why it's working currently (and not causing any problems I'm aware of) is b/c cetl::rtti_cast<> implementation is not requiring currently to inherit from cetl::rtti interface - @pavel-kirienko Pavel, is it a feature or a bug? ;) IMO on one hand it could be a feature, namely:

On the other hand, it is probably not good that we have rtti interface defined but don't enforce it properly... f.e. if I add to the interface a new pure virtual method then I would expect to see compilation failures about classes being abstract...

pavel-kirienko commented 1 week ago

That interface is mostly needed for the rtti_helper, so consider it a feature. We should implement that interface at least for regularity, though (making it final to avoid vtable lookup overheads where possible).