ros-controls / ros_control

Generic and simple controls framework for ROS
http://wiki.ros.org/ros_control
BSD 3-Clause "New" or "Revised" License
470 stars 307 forks source link

Add function specifiers and modernize constructors #430

Closed matthew-reynolds closed 4 years ago

matthew-reynolds commented 4 years ago

Part of #403.

Add function specifiers (override, noexcept) and cleanup ctors & dtors (= default, = delete, and removing unnecessary).

A couple of notes:


This was mostly done using clang-tidy:

Checks: '-*,
        modernize-use-equals-default,
        modernize-use-equals-delete,
        modernize-use-override,
        modernize-use-noexcept
        '
matthew-reynolds commented 4 years ago

A tiny bit unsure whether without declaring any, not even an override, derived class destructors will be virtual too

Yep, the derived class destructor will be virtual if there's a virtual destructor anywhere in the inheritance chain. As a quick sanity check: http://cpp.sh/4d4l

Whether we should keep the virtual destructor as a reminder for people using this class to define one. Yes this class gets it from it's parent but this class is meant to be subclassed from so I think we should not rely on implicit virtuals.

Hmm, I maybe see the argument for keeping it to make it extra clear that the class has a virtual destructor, but I don't think it's really important to tell that to the author of subclasses. They don't even need to be aware of the virtual dtor, since it doesn't affect how they write their code - If they need a destructor to do some cleanup for stuff in their class, they can add one, but if they don't need one, they don't need to add one. I think it's unnecessary to "remind people to define one" since not all subclasses will require a user-defined destructor, and they can just follow the regular rules about when to define one.

I also think it's still pretty clear that CombinedRobotHW has a virtual destructor, since RobotHW has one. Similarly, that Controller has a virtual destructor, since ControllerBase has one.

bmagyar commented 4 years ago

It wasn't hard to convince me ;) Keeping virtual as self-documentation to people writing derived classes is already somewhat disrupted by override where using virtual is superfluous.

jordan-palacios commented 4 years ago

Out of curiosity what's the criteria for defaulting constructors instead of removing them?

For example, as far as I can tell, this one could be removed too.

matthew-reynolds commented 4 years ago

Which one is that link pointing to?

The rationale was to remove as many as possible, and then only default ones that have other overloads. If there are any left that can be removed, they're probably just a mistake or oversight. Happy to update the PR.

(I would also like all those ctor overloads to be explicit too, but that will have to target Noetic since it's breaking. See CppCoreGuidelines C.46 and https://abseil.io/tips/142. That's on my list of follow-ups)

bmagyar commented 4 years ago

Rebased for latest melodic, ready to merge once CI is happy