eProsima / Fast-DDS-Gen

Fast-DDS IDL code generator tool. Looking for commercial support? Contact info@eprosima.com
Apache License 2.0
77 stars 59 forks source link

Use virtual or override, not both #213

Closed jwillemsen closed 10 months ago

jwillemsen commented 11 months ago

Is there an already existing issue for this?

Expected behavior

I noticed at https://github.com/eProsima/Fast-DDS/blob/107ea8d64942102696840cd7d3e4cf93fa7a143e/test/unittest/dynamic_types/idl/new_features_4_2PubSubTypes.h#L52 that you are using virtual and override here. Lot of tools do trigger a warning on that, when you are using override, you shouldn't use virtual at that place, see https://en.cppreference.com/w/cpp/language/override

Current behavior

eProsima_user_DllExport virtual bool serialize(
            void* data,
            eprosima::fastrtps::rtps::SerializedPayload_t* payload) override;

Should be

eProsima_user_DllExport bool serialize(
            void* data,
            eprosima::fastrtps::rtps::SerializedPayload_t* payload) override;

Steps to reproduce

Code review

Fast DDS version/commit

Master

Platform/Architecture

Ubuntu Focal 20.04 arm64

Transport layer

Default configuration, UDPv4 & SHM

Additional context

No response

XML configuration file

No response

Relevant log output

No response

Network traffic capture

No response

JLBuenoLopez commented 11 months ago

Thanks for your contribution @jwillemsen

I have moved the ticket to Fast DDS-Gen repository as the mentioned code is generated by this tool. Also #208 is already fixing the code you mention. I have taken a look and it seems that with changes in that PR, the only method which is still using virtual and override at the same time is the PubSubTypes destructor.

jwillemsen commented 11 months ago

Also for destructors only virtual is necessary for the base class, any derived can just use override. I would also recommend to have a look at default for all copy/assignment operators, reduces the generated code heavily