Add a KeepVirtual option to misc-use-override

[• ehsan]

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.

[alexfh]

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?

[alexfh]

What's the status of this patch?

[• ehsan]

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?

[alexfh]

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 ;)

clang-tidy/misc/UseOverrideCheck.h

 namespace clang {
 namespace tidy {
 namespace misc {
 
 /// \brief Use C++11's 'override' and remove 'virtual' where applicable.
 class UseOverrideCheck : public ClangTidyCheck {
 public:
   UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+      : ClangTidyCheck(Name, Context),
+        KeepVirtual(Options.get("KeepVirtual", false)) {}
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool KeepVirtual;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USEOVERRIDECHECK_H

clang-tidy/misc/UseOverrideCheck.cpp

 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
+void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "KeepVirtual", KeepVirtual);
+}
+
 void UseOverrideCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(methodDecl(isOverride()).bind("method"), this);
 }
 
 // Re-lex the tokens to get precise locations to insert 'override' and remove
 // 'virtual'.
 static SmallVector<Token, 16>
 ParseTokens(CharSourceRange Range, const MatchFinder::MatchResult &Result) {
void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
   unsigned KeywordCount = HasVirtual + HasOverride + HasFinal;
 
   if (!OnlyVirtualSpecified && KeywordCount == 1)
     return; // Nothing to do.
 
   std::string Message;
 
   if (OnlyVirtualSpecified) {
     Message =
         "prefer using 'override' or (rarely) 'final' instead of 'virtual'";
   } else if (KeywordCount == 0) {
     Message = "annotate this function with 'override' or (rarely) 'final'";
   } else {
     StringRef Redundant =
-        HasVirtual ? (HasOverride && HasFinal ? "'virtual' and 'override' are"
-                                              : "'virtual' is")
-                   : "'override' is";
+        (!KeepVirtual && HasVirtual)
+            ? (HasOverride && HasFinal ? "'virtual' and 'override' are"
+            : (HasOverride && HasFinal ? "'override' is" : "");
     StringRef Correct = HasFinal ? "'final'" : "'override'";
 
+    if (!Redundant.size())
+      return;
+
     Message =
         (llvm::Twine(Redundant) +
          " redundant since the function is already declared " + Correct).str();
   }
 
   DiagnosticBuilder Diag = diag(Method->getLocation(), Message);
 
   CharSourceRange FileRange = Lexer::makeFileCharRange(
void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
   }
 
   if (HasFinal && HasOverride) {
     SourceLocation OverrideLoc = Method->getAttr<OverrideAttr>()->getLocation();
     Diag << FixItHint::CreateRemoval(
         CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc));
   }
 
-  if (HasVirtual) {
+  if (!KeepVirtual && HasVirtual) {
     for (Token Tok : Tokens) {
       if (Tok.is(tok::kw_virtual)) {
         Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
             Tok.getLocation(), Tok.getLocation()));
         break;
       }
     }
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

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

+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-use-override %t -config="{CheckOptions: [{key: misc-use-override.KeepVirtual, value: 1}]}" -- -std=c++11
+// REQUIRES: shell
+
+// This test is based on misc-use-override.cpp.
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct MUST_USE_RESULT MustUseResultObject {};
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  virtual void d2();
+  virtual void e() = 0;
+  virtual void f() = 0;
+  virtual void g() = 0;
+
+  virtual void j() const;
+  virtual MustUseResultObject k();
+  virtual bool l() MUST_USE_RESULT UNUSED;
+  virtual bool n() MUST_USE_RESULT UNUSED;
+
+  virtual void m();
+  virtual void m2();
+  virtual void o() __attribute__((unused));
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual'
+  // CHECK-FIXES: {{^}}  virtual ~SimpleCases() override;
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this
+  // CHECK-FIXES: {{^}}  void a() override;
+
+  void b() override;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  void b() override;
+
+  virtual void c();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void c() override;
+
+  virtual void d() override;
+  // ChECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual void d() override;
+
+  virtual void d2() final;
+  // ChECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual void d2() final;
+
+  virtual void e() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void e() override = 0;
+
+  virtual void f()=0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void f()override =0;
+
+  virtual void g() ABSTRACT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void g() override ABSTRACT;
+
+  virtual void j() const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void j() const override;
+
+  virtual MustUseResultObject k();  // Has an implicit attribute.
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual MustUseResultObject k() override;
+
+  virtual bool l() MUST_USE_RESULT UNUSED;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual bool l() override MUST_USE_RESULT UNUSED;
+
+  virtual bool n() UNUSED MUST_USE_RESULT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual bool n() override UNUSED MUST_USE_RESULT;
+
+  void m() override final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: 'override' is redundant since the function is already declared 'final'
+  // CHECK-FIXES: {{^}}  void m() final;
+
+  virtual void m2() override final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'override' is redundant since the function is already declared 'final'
+  // CHECK-FIXES: {{^}}  virtual void m2() final;
+
+  virtual void o() __attribute__((unused));
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void o() override __attribute__((unused));
+};
+
+// CHECK-MESSAGES-NOT: warning:
+
+void SimpleCases::c() {}
+// CHECK-FIXES: {{^}}void SimpleCases::c() {}
+
+SimpleCases::~SimpleCases() {}
+// CHECK-FIXES: {{^}}SimpleCases::~SimpleCases() {}
+
+struct DefaultedDestructor : public Base {
+  DefaultedDestructor() {}
+  virtual ~DefaultedDestructor() = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual ~DefaultedDestructor() override = default;
+};
+
+struct FinalSpecified : public Base {
+public:
+  virtual ~FinalSpecified() final;
+  // ChECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual ~FinalSpecified() final;
+
+  void b() final;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  void b() final;
+
+  virtual void d() final;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual void d() final;
+
+  virtual void e() final = 0;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual void e() final = 0;
+
+  virtual void j() const final;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual void j() const final;
+
+  virtual bool l() final MUST_USE_RESULT UNUSED;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual bool l() final MUST_USE_RESULT UNUSED;
+};
+
+struct InlineDefinitions : public Base {
+public:
+  virtual ~InlineDefinitions() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual ~InlineDefinitions() override {}
+
+  void a() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this
+  // CHECK-FIXES: {{^}}  void a() override {}
+
+  void b() override {}
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  void b() override {}
+
+  virtual void c() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void c() override {}
+
+  virtual void d() override {}
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual void d() override {}
+
+  virtual void j() const {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void j() const override {}
+
+  virtual MustUseResultObject k() {}  // Has an implicit attribute.
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual MustUseResultObject k() override {}
+
+  virtual bool l() MUST_USE_RESULT UNUSED {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual bool l() override MUST_USE_RESULT UNUSED {}
+};
+
+struct Macros : public Base {
+  // Tests for 'virtual' and 'override' being defined through macros. Basically
+  // give up for now.
+  NOT_VIRTUAL void a() NOT_OVERRIDE;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: annotate this
+  // CHECK-FIXES: {{^}}  NOT_VIRTUAL void a() override NOT_OVERRIDE;
+
+  VIRTUAL void b() NOT_OVERRIDE;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  VIRTUAL void b() override NOT_OVERRIDE;
+
+  NOT_VIRTUAL void c() OVERRIDE;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  NOT_VIRTUAL void c() OVERRIDE;
+
+  VIRTUAL void d() OVERRIDE;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  VIRTUAL void d() OVERRIDE;
+
+#define FUNC(return_type, name) return_type name()
+  FUNC(void, e);
+  // CHECK-FIXES: {{^}}  FUNC(void, e);
+
+#define F virtual void f();
+  F
+  // CHECK-FIXES: {{^}}  F
+
+  VIRTUAL void g() OVERRIDE final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'override' is redundant since the function is already declared 'final'
+  // CHECK-FIXES: {{^}}  VIRTUAL void g() final;
+};
+
+// Tests for templates.
+template <typename T> struct TemplateBase {
+  virtual void f(T t);
+};
+
+template <typename T> struct DerivedFromTemplate : public TemplateBase<T> {
+  virtual void f(T t);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual void f(T t) override;
+};
+void f() { DerivedFromTemplate<int>().f(2); }
+
+template <class C>
+struct UnusedMemberInstantiation : public C {
+  virtual ~UnusedMemberInstantiation() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using
+  // CHECK-FIXES: {{^}}  virtual ~UnusedMemberInstantiation() override {}
+};
+struct IntantiateWithoutUse : public UnusedMemberInstantiation<Base> {};
+
+struct Base2 {
+  virtual ~Base2() {}
+  virtual void a();
+};
+
+// The OverrideAttr isn't propagated to specializations in all cases. Make sure
+// we don't add "override" a second time.
+template <int I>
+struct MembersOfSpecializations : public Base2 {
+  void a() override;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  void a() override;
+};
+template <> void MembersOfSpecializations<3>::a() {}
+void ff() { MembersOfSpecializations<3>().a(); };