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

32

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

41

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

1

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

32

Changed order of the first two checks.

41

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

1

Done.

[djasper]

Updated patch.

[alexfh]

clang-tidy/misc/UseOverride.cpp

41

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

41

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);
+}
+
+void UseOverride::check(const MatchFinder::MatchResult &Result) {
+  const FunctionDecl *Method = Result.Nodes.getStmtAs<FunctionDecl>("method");
+  assert(Method != nullptr);
+  if (Method->isImplicit() || Method->getLocation().isMacroID() ||
+      Method->isOutOfLine())
+    return;
+
+  if (Method->getInstantiatedFromMemberFunction() != nullptr)
+    Method = Method->getInstantiatedFromMemberFunction();
+
+  if (Method->getAttr<clang::OverrideAttr>() != nullptr &&
+      !Method->isVirtualAsWritten())
+    return; // Nothing to do.
+
+  DiagnosticBuilder Diag = diag(Method->getLocation(),
+                                "Prefer using 'override' instead of 'virtual'");
+
+  // Re-lex the tokens to get precise locations to insert 'override' and remove
+  // 'virtual'.
+  std::pair<FileID, unsigned> LocInfo =
+      Result.SourceManager->getDecomposedLoc(Method->getLocStart());
+  StringRef File = Result.SourceManager->getBufferData(LocInfo.first);
+  const char *TokenBegin = File.data() + LocInfo.second;
+  Lexer RawLexer(Result.SourceManager->getLocForStartOfFile(LocInfo.first),
+                 Result.Context->getLangOpts(), File.begin(), TokenBegin,
+                 File.end());
+  SmallVector<Token, 16> Tokens;
+  SmallVector<StringRef, 16> TokenText;
+  Token Tok;
+  while (!RawLexer.LexFromRawLexer(Tok)) {
+    if (Tok.is(tok::semi) || Tok.is(tok::l_brace))
+      break;
+    if (Result.SourceManager->isBeforeInTranslationUnit(Method->getLocEnd(),
+                                                        Tok.getLocation()))
+      break;
+    Tokens.push_back(Tok);
+    TokenText.push_back(
+        StringRef(Result.SourceManager->getCharacterData(Tok.getLocation()),
+                  Tok.getLength()));
+  }
+
+  // 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 =
+              Result.SourceManager->getExpansionLoc(attr->getLocation());
+        }
+      }
+    } else if (Method->doesThisDeclarationHaveABody()) {
+      InsertLoc = Method->getBody()->getLocStart();
+    }
+
+    if (!InsertLoc.isValid()) {
+      if (Tokens.size() > 2 && TokenText.back() == "0" &&
+          TokenText[Tokens.size() - 2] == "=") {
+        InsertLoc =
+            Tokens[Tokens.size() - 2].getLocation();
+      } else if (TokenText.back() == "ABSTRACT") {
+        InsertLoc = Tokens.back().getLocation();
+      }
+    }
+
+    if (!InsertLoc.isValid()) {
+      InsertLoc = clang::Lexer::getLocForEndOfToken(
+          Method->getLocEnd(), 0, *Result.SourceManager,
+          Result.Context->getLangOpts());
+      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) && TokenText[i] == "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
+
+struct 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 h();
+  virtual void i();
+};
+
+struct SimpleCases : public Base {
+public:
+  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 h() {}
+  // CHECK: {{^  void h\(\) override {}}}
+
+  virtual void i();
+  // CHECK: {{^  void i\(\) override;}}
+};
+
+void SimpleCases::i() {}
+// CHECK: {{^void SimpleCases::i\(\) {}}}