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

Remove virtual function specifiers where override is used #1458

Conversation

Projects
3 participants
@wezrule
Copy link
Collaborator

commented Dec 15, 2018

Virtual function specifiers have always been implicitly inherited from their base classes. So the use of the virtual keyword in these cases has been redundant but was recommended in pre-modern C++ so that it was more easily distinguishable from non-virtual functions when reading derived class definitions. However since C++11, the override specifier has superseded this with compile time safety, and so explicitly writing both in a derived class is not required.

I propose that the virtual specifier is removed in any function declaration where both the override and virtual function specifiers are used.

The following command was used in the root directory to find and replace all instances where virtual and override specifiers were used.
find -type f -print0 | xargs -0 sed -ri 's/(virtual\s+)(.*override)/\2/g'

There were 2 files which were affected, one was rai/lib/expected.hpp but as this is third party code it has been left as is.

This change has not checked whether there are places override should be used where it is currently not.

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2018

Agreed on this change. Is this something that would be picked up by clang-tidy --modernize? We should probably have that in the pipeline in addition to clang-format just to catch coding practices.

@wezrule

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 15, 2018

I'm unfamiliar with both these clang tools. It looks like there is clang-tidy --modernize-use-override:

https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html
"Use C++11’s override and remove virtual where applicable."

Having something like that in the pipeline would definitely be beneficial

@clemahieu clemahieu merged commit c54fc90 into nanocurrency:master Dec 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rkeene rkeene added this to the V17.0 milestone Dec 16, 2018

@rkeene rkeene modified the milestones: V17.0, V18.0 Dec 16, 2018

@wezrule wezrule deleted the wezrule:remove_redundant_virtual_function_specifiers branch Dec 16, 2018

@zhyatt zhyatt added this to Unscheduled in V18 Dec 27, 2018

@zhyatt zhyatt moved this from Unscheduled to CP 0 in V18 Jan 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.