Initial version of clang-tidy check to use override instead of virual.

[djasper]

Summary

I'll add some more tests for members of template classes etc., but I wanted to get some initial feedback.

[alexfh]

This check (especially the fix part) would be really nice to have.

clang-tidy/misc/UseOverride.cpp

33

Does it make sense to check for getLocation().isMacroID() after this?

42

Can you adapt FindToken from google/GoogleTidyModule.cpp to be usable here (and move it to a common place)? (Or maybe convert it to ParseTokens or something similar.)

test/clang-tidy/use-override.cpp

2

Please add tests with macros: when the whole class is defined in a macro and when a macro is used for "override" and for "virtual" (which is interesting, as the code removes "virtual").

[djasper]

clang-tidy/misc/UseOverride.cpp

33

Changed order of the first two checks.

42

Only partially as I need to lex all the tokens of the function definition (to scan backwards for "= 0"). I guess we could pull out abstraction here, but I am not exactly sure what that should be.

test/clang-tidy/use-override.cpp

2

Done.

[djasper]

Updated patch.

[alexfh]

clang-tidy/misc/UseOverride.cpp

42

We could at least pull out something like this:

vector<Token> ParseTokens(SourceLocation From, SourceLocation To, <SourceManager and whatever else is needed>);

To be more generic "To" could be replaced by a predicate, though this may be too generic ;)

97

Instead of special-casing this macro, you could find if the identifier is a macro, and see what it expands to using something similar to this:

IdentifierInfo *II = PP.getIdentifierInfo(Token.back());
if (II.hadMacroDefinition()) {
  if (const DefMacroDirective *Def = PP.getMacroDirectiveHistory(II)->findDirectiveAtLoc(...)->getDirective()) {
    <compare Def->getInfo()->tokens_{begin,end}() with {'=', '0'}>
...

[djasper]

clang-tidy/misc/UseOverride.cpp

42

Done. Left locally for now, but we can move it and make it public if necessary.

97

Clang-tidy doesn't have access to the preprocessor. However, I think we'll want to store the corresponding locations explicitly in FunctionDecls. Left FIXME to this effect.

[djasper]

Address comments and add more tests.

[alexfh]

Looks good once you remove " Method->dump();" that you left on line 55.

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   MiscTidyModule.cpp
   RedundantSmartptrGet.cpp
+  UseOverride.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangTidy
   )

clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.cpp - clang-tidy ----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "RedundantSmartptrGet.h"
+#include "UseOverride.h"
 
 namespace clang {
 namespace tidy {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.addCheckFactory(
         "misc-argument-comment",
         new ClangTidyCheckFactory<ArgumentCommentCheck>());
     CheckFactories.addCheckFactory(
         "misc-redundant-smartptr-get",
         new ClangTidyCheckFactory<RedundantSmartptrGet>());
+    CheckFactories.addCheckFactory(
+        "misc-use-override",
+        new ClangTidyCheckFactory<UseOverride>());
   }
 };
 
 // Register the MiscTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<MiscModule>
 X("misc-module", "Adds miscellaneous lint checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the MiscModule.
 volatile int MiscModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/misc/UseOverride.h

+//===--- UseOverride.h - clang-tidy -----------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USE_OVERRIDE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USE_OVERRIDE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Use C++11's 'override' and remove 'virtual' where applicable.
+class UseOverride : public ClangTidyCheck {
+public:
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_USE_OVERRIDE_H
+

clang-tidy/misc/UseOverride.cpp

+//===--- UseOverride.cpp - clang-tidy -------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseOverride.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void UseOverride::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 SourceManager &Sources,
+                                          LangOptions LangOpts) {
+  std::pair<FileID, unsigned> LocInfo =
+      Sources.getDecomposedLoc(Range.getBegin());
+  StringRef File = Sources.getBufferData(LocInfo.first);
+  const char *TokenBegin = File.data() + LocInfo.second;
+  Lexer RawLexer(Sources.getLocForStartOfFile(LocInfo.first), LangOpts,
+                 File.begin(), TokenBegin, File.end());
+  SmallVector<Token, 16> Tokens;
+  Token Tok;
+  while (!RawLexer.LexFromRawLexer(Tok)) {
+    if (Tok.is(tok::semi) || Tok.is(tok::l_brace))
+      break;
+    if (Sources.isBeforeInTranslationUnit(Range.getEnd(), Tok.getLocation()))
+      break;
+    Tokens.push_back(Tok);
+  }
+  return Tokens;
+}
+
+static StringRef GetText(const Token& Tok, const SourceManager &Sources) {
+  return {Sources.getCharacterData(Tok.getLocation()), Tok.getLength()};
+}
+
+void UseOverride::check(const MatchFinder::MatchResult &Result) {
+  const FunctionDecl *Method = Result.Nodes.getStmtAs<FunctionDecl>("method");
+  const SourceManager &Sources = *Result.SourceManager;
+
+  Method->dump();
+
+  assert(Method != nullptr);
+  if (Method->getInstantiatedFromMemberFunction() != nullptr)
+    Method = Method->getInstantiatedFromMemberFunction();
+
+  if (Method->isImplicit() || Method->getLocation().isMacroID() ||
+      Method->isOutOfLine())
+    return;
+
+  if (Method->getAttr<clang::OverrideAttr>() != nullptr &&
+      !Method->isVirtualAsWritten())
+    return; // Nothing to do.
+
+  DiagnosticBuilder Diag = diag(Method->getLocation(),
+                                "Prefer using 'override' instead of 'virtual'");
+
+  CharSourceRange FileRange =
+      Lexer::makeFileCharRange(CharSourceRange::getTokenRange(
+                                   Method->getLocStart(), Method->getLocEnd()),
+                               Sources, Result.Context->getLangOpts());
+
+  if (!FileRange.isValid())
+    return;
+
+  // FIXME: Instead of re-lexing and looking for specific macros such as
+  // 'ABSTRACT', properly store the location of 'virtual' and '= 0' in each
+  // FunctionDecl.
+  SmallVector<Token, 16> Tokens = ParseTokens(FileRange, Sources,
+                                              Result.Context->getLangOpts());
+
+  // Add 'override' on inline declarations that don't already have it.
+  if (Method->getAttr<clang::OverrideAttr>() == nullptr) {
+    SourceLocation InsertLoc;
+    StringRef ReplacementText = "override ";
+
+    if (Method->hasAttrs()) {
+      for (const clang::Attr *attr : Method->getAttrs()) {
+        if (!attr->isImplicit()) {
+          InsertLoc = Sources.getExpansionLoc(attr->getLocation());
+          break;
+        }
+      }
+    }
+
+    if (InsertLoc.isInvalid() && Method->doesThisDeclarationHaveABody()) {
+      InsertLoc = Method->getBody()->getLocStart();
+    }
+
+    if (!InsertLoc.isValid()) {
+      if (Tokens.size() > 2 && GetText(Tokens.back(), Sources) == "0" &&
+          GetText(Tokens[Tokens.size() - 2], Sources) == "=") {
+        InsertLoc = Tokens[Tokens.size() - 2].getLocation();
+      } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {
+        InsertLoc = Tokens.back().getLocation();
+      }
+    }
+
+    if (!InsertLoc.isValid()) {
+      InsertLoc = FileRange.getEnd();
+      ReplacementText = " override";
+    }
+    Diag << FixItHint::CreateInsertion(InsertLoc, ReplacementText);
+  }
+
+  if (Method->isVirtualAsWritten()) {
+    for (unsigned i = 0, e = Tokens.size(); i != e; ++i) {
+      if (Tokens[i].is(tok::raw_identifier) &&
+          GetText(Tokens[i], Sources) == "virtual") {
+        Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+            Tokens[i].getLocation(), Tokens[i].getLocation()));
+        break;
+      }
+    }
+  }
+}
+
+} // namespace tidy
+} // namespace clang

test/clang-tidy/check_clang_tidy_fix.sh

 #!/bin/sh
 #
 # Run clang-tidy in fix mode and verify the result.
 
 INPUT_FILE=$1
 CHECK_TO_RUN=$2
 TEMPORARY_FILE=$3.cpp
 
 grep -Ev "// *[A-Z-]+:" ${INPUT_FILE} > ${TEMPORARY_FILE}
 clang-tidy ${TEMPORARY_FILE} -fix --checks=${CHECK_TO_RUN} \
     --disable-checks="" -- --std=c++11
-FileCheck -input-file=${TEMPORARY_FILE} ${INPUT_FILE}
+FileCheck -input-file=${TEMPORARY_FILE} ${INPUT_FILE} -strict-whitespace

test/clang-tidy/use-override.cpp

+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-use-override %t
+// REQUIRES: shell
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+
+struct MUST_USE_RESULT MustUseResultObject {};
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  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;
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK: {{^  ~SimpleCases\(\) override;}}
+
+  void a();
+  // CHECK: {{^  void a\(\) override;}}
+  void b() override;
+  // CHECK: {{^  void b\(\) override;}}
+  virtual void c();
+  // CHECK: {{^  void c\(\) override;}}
+  virtual void d() override;
+  // CHECK: {{^  void d\(\) override;}}
+
+  virtual void e() = 0;
+  // CHECK: {{^  void e\(\) override = 0;}}
+  virtual void f()=0;
+  // CHECK: {{^  void f\(\)override =0;}}
+  virtual void g() ABSTRACT;
+  // CHECK: {{^  void g\(\) override ABSTRACT;}}
+
+  virtual void j() const;
+  // CHECK: {{^  void j\(\) const override;}}
+  virtual MustUseResultObject k();  // Has an implicit attribute.
+  // CHECK: {{^  MustUseResultObject k\(\) override;}}
+  virtual bool l() MUST_USE_RESULT; // Has an explicit attribute
+  // CHECK: {{^  bool l\(\) override MUST_USE_RESULT;}}
+};
+
+void SimpleCases::i() {}
+// CHECK: {{^void SimpleCases::i\(\) {}}}
+
+struct InlineDefinitions : public Base {
+public:
+  virtual ~InlineDefinitions() {}
+  // CHECK: {{^  ~InlineDefinitions\(\) override {}}}
+
+  void a() {}
+  // CHECK: {{^  void a\(\) override {}}}
+  void b() override {}
+  // CHECK: {{^  void b\(\) override {}}}
+  virtual void c() {}
+  // CHECK: {{^  void c\(\) override {}}}
+  virtual void d() override {}
+  // CHECK: {{^  void d\(\) override {}}}
+
+  virtual void j() const {}
+  // CHECK: {{^  void j\(\) const override {}}}
+  virtual MustUseResultObject k() {}  // Has an implicit attribute.
+  // CHECK: {{^  MustUseResultObject k\(\) override {}}}
+  virtual bool l() MUST_USE_RESULT {} // Has an explicit attribute
+  // CHECK: {{^  bool l\(\) override MUST_USE_RESULT {}}}
+};
+
+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: {{^  NOT_VIRTUAL void a\(\) override NOT_OVERRIDE;}}
+
+  VIRTUAL void b() NOT_OVERRIDE;
+  // CHECK: {{^  VIRTUAL void b\(\) override NOT_OVERRIDE;}}
+
+  NOT_VIRTUAL void c() OVERRIDE;
+  // CHECK: {{^  NOT_VIRTUAL void c\(\) OVERRIDE;}}
+
+  VIRTUAL void d() OVERRIDE;
+  // CHECK: {{^  VIRTUAL void d\(\) OVERRIDE;}}
+
+#define FUNC(name, return_type) return_type name()
+  FUNC(void, e);
+  // CHECK: {{^  FUNC\(void, e\);}}
+
+#define F virtual void f();
+  F
+  // CHECK: {{^  F}}
+};
+
+// 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: {{^  void f\(T t\) override;}}
+};
+void f() { DerivedFromTemplate<int>().f(2); }
+
+template <class C>
+struct UnusedMemberInstantiation : public C {
+  virtual ~UnusedMemberInstantiation() {}
+  // CHECK: {{^  ~UnusedMemberInstantiation\(\) override {}}}
+};
+struct IntantiateWithoutUse : public UnusedMemberInstantiation<Base> {};
+
+// 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 Base {
+  void a() override;
+  // CHECK: {{^  void a\(\) override;}}
+};
+template <> void MembersOfSpecializations<3>::a() {}
+void f() { D<3>().a(); };