Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add function specifiers and modernize constructors #430

Merged

Conversation

matthew-reynolds
Copy link
Member

Part of #403.

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

A couple of notes:

  • Per CppCoreGuidelines C.128, Virtual functions should specify exactly one of virtual, override, or final. Therefore, lots of virtual specifiers were removed in favour of overrides.
  • We had a lot of unnecessary empty constructors and overriding destructors. Wherever possible, these have been removed.
  • throw() -> noexcept. I did not add any additional noexcepts though, only updated the existing ones in the custom exceptions.
  • ImuSensorHandle::Data::Data() must be user-defined, it cannot be = default. See https://stackoverflow.com/a/17436088/1932358

This was mostly done using clang-tidy:

Checks: '-*,
        modernize-use-equals-default,
        modernize-use-equals-delete,
        modernize-use-override,
        modernize-use-noexcept
        '

@matthew-reynolds matthew-reynolds changed the title Function specifiers Add function specifiers and modernize constructors Mar 15, 2020
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it looks very good to me, love to see code improvements with a negative balance sheet ;)
I am unsure about the virtual destructors though...

  • A tiny bit unsure whether without declaring any, not even an override, derived class destructors will be virtual too
  • Whether we should keep the virtual desctructor 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.

@matthew-reynolds
Copy link
Member Author

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
Copy link
Member

bmagyar commented Mar 16, 2020

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
Copy link
Member

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
Copy link
Member Author

matthew-reynolds commented Apr 7, 2020

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 bmagyar changed the base branch from melodic-devel to noetic-devel April 23, 2020 07:50
@bmagyar bmagyar changed the base branch from noetic-devel to melodic-devel April 23, 2020 07:50
@bmagyar bmagyar mentioned this pull request Apr 23, 2020
5 tasks
@bmagyar bmagyar changed the base branch from melodic-devel to noetic-devel May 9, 2020 12:41
@bmagyar bmagyar changed the base branch from noetic-devel to melodic-devel May 9, 2020 16:01
@bmagyar
Copy link
Member

bmagyar commented May 9, 2020

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

@bmagyar bmagyar merged commit bf58ff3 into ros-controls:melodic-devel May 9, 2020
@matthew-reynolds matthew-reynolds deleted the function-specifiers branch June 8, 2020 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants