Page MenuHomePhabricator

Add a KeepVirtual option to misc-use-override
AbandonedPublic

Authored by ehsan on Apr 26 2015, 5:43 PM.

Details

Reviewers
alexfh
Summary

Currently, clang-tidy will complain about the virtual keyword for
override and final methods. Some coding styles such as Mozilla doesn't
prohibit the usage of the virtual keyword.

This patch adds a KeepVirtual option for the misc-use-override check to
instruct it to ignore existing virtual keywords.

Diff Detail

Event Timeline

ehsan updated this revision to Diff 24448.Apr 26 2015, 5:43 PM
ehsan retitled this revision from to Add a KeepVirtual option to misc-use-override.
ehsan updated this object.
ehsan edited the test plan for this revision. (Show Details)
ehsan added a reviewer: alexfh.
ehsan added a subscriber: Unknown Object (MLST).
alexfh edited edge metadata.May 8 2015, 4:06 AM

Some coding styles such as Mozilla doesn't prohibit the usage of the virtual keyword.

Apparently, Mozilla style guide even mandates the use of both virtual and override on overridden methods:

Method declarations that override virtual methods from a base class should be annotated with both "virtual" and "override".

I find this rule weird and if I worked with Mozilla code, I'd better spend time to fix the style guide. But I don't so we can add this flag. See a few comments inline.

clang-tidy/misc/UseOverrideCheck.cpp
92

In case KeepVirtual is true, the message will be inconsistent with the suggested fix. Namely, the "instead of 'virtual'" part.

104

nit: s/!Redundant.size()/Redundant.empty()/

test/clang-tidy/misc-use-override-keep-virtual.cpp
1

This file should be svn/git cped from the original test. Or is just Phabricator failing to recognize this and display the diff appropriately?

What's the status of this patch?

Sorry, I kind of dropped the ball here!

At Mozilla we ended up deciding on allowing only a maximum of one of virtual, final or override per function declaration, similar to the Google coding style, so we won't need the KeepVirtual option. Should I still add it nevertheless?

In D9285#224690, @ehsan wrote:

Sorry, I kind of dropped the ball here!

At Mozilla we ended up deciding on allowing only a maximum of one of virtual, final or override per function declaration, similar to the Google coding style, so we won't need the KeepVirtual option. Should I still add it nevertheless?

No, I don't think the option (and the rule it backs up) makes sense. I'm glad you got rid of the relevant style rule ;)

ehsan abandoned this revision.Aug 18 2015, 5:44 PM