Bug 1324320 - Port the must-override check to the clang-tidy compatible framework; r=mystor

build/clang-plugin/BaseCheck.h

@@ -2,16 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-#ifndef BaseCheck_h__
-#define BaseCheck_h__
+#ifndef MozCheck_h__
+#define MozCheck_h__
 
 class MozContext {};
 typedef MozContext ContextType;
 
-class BaseCheck : public MatchFinder::MatchCallback {
+class MozCheck : public MatchFinder::MatchCallback {
 public:
-  BaseCheck(StringRef CheckName, ContextType* Context) {}
+  MozCheck(StringRef CheckName, ContextType* Context) {}
   virtual void registerMatchers(MatchFinder *Finder) {}
+  virtual void registerPPCallbacks(CompilerInstance& CI) {}
   virtual void check(const MatchFinder::MatchResult &Result) {}
   DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
                          DiagnosticIDs::Level Level = DiagnosticIDs::Warning) {

build/clang-plugin/Checks.inc

@@ -9,6 +9,7 @@ CHECK(AssertAssignmentChecker, "assignment-in-assert")
 CHECK(ExplicitImplicitChecker, "implicit-constructor")
 CHECK(ExplicitOperatorBoolChecker, "explicit-operator-bool")
 CHECK(KungFuDeathGripChecker, "kungfu-death-grip")
+CHECK(MustOverrideChecker, "must-override")
 CHECK(NaNExprChecker, "nan-expr")
 CHECK(NeedsNoVTableTypeChecker, "needs-no-vtable-type")
 CHECK(NoAddRefReleaseOnReturnChecker, "no-addref-release-on-return")

build/clang-plugin/ChecksIncludes.inc

@@ -10,6 +10,7 @@
 #include "ExplicitImplicitChecker.h"
 #include "ExplicitOperatorBoolChecker.h"
 #include "KungFuDeathGripChecker.h"
+#include "MustOverrideChecker.h"
 #include "NaNExprChecker.h"
 #include "NeedsNoVTableTypeChecker.h"
 #include "NoAddRefReleaseOnReturnChecker.h"

build/clang-plugin/DiagnosticsMatcher.cpp

@@ -4,13 +4,14 @@
 
 #include "DiagnosticsMatcher.h"
 
-DiagnosticsMatcher::DiagnosticsMatcher() :
+DiagnosticsMatcher::DiagnosticsMatcher(CompilerInstance& CI) :
 #define CHECK(cls, name) cls ## _(name),
 #include "Checks.inc"
 #undef CHECK
   AstMatcher()
 {
-#define CHECK(cls, name) cls ## _.registerMatchers(&AstMatcher);
+#define CHECK(cls, name) cls ## _.registerMatchers(&AstMatcher); \
+                         cls ## _.registerPPCallbacks(CI);
 #include "Checks.inc"
 #undef CHECK
 }

build/clang-plugin/DiagnosticsMatcher.h

@@ -9,7 +9,7 @@
 
 class DiagnosticsMatcher {
 public:
-  DiagnosticsMatcher();
+  DiagnosticsMatcher(CompilerInstance& CI);
 
   ASTConsumerPtr makeASTConsumer() { return AstMatcher.newASTConsumer(); }
 

build/clang-plugin/MozChecker.cpp

@@ -38,64 +38,6 @@ void MozChecker::handleUnusedExprResult(const Stmt *Statement) {
   }
 }
 
-bool MozChecker::VisitCXXRecordDecl(CXXRecordDecl *D) {
-  // We need definitions, not declarations
-  if (!D->isThisDeclarationADefinition())
-    return true;
-
-  // Look through all of our immediate bases to find methods that need to be
-  // overridden
-  typedef std::vector<CXXMethodDecl *> OverridesVector;
-  OverridesVector MustOverrides;
-  for (CXXRecordDecl::base_class_iterator Base = D->bases_begin(),
-                                          E = D->bases_end();
-       Base != E; ++Base) {
-    // The base is either a class (CXXRecordDecl) or it's a templated class...
-    CXXRecordDecl *Parent = Base->getType()
-                                .getDesugaredType(D->getASTContext())
-                                ->getAsCXXRecordDecl();
-    // The parent might not be resolved to a type yet. In this case, we can't
-    // do any checking here. For complete correctness, we should visit
-    // template instantiations, but this case is likely to be rare, so we will
-    // ignore it until it becomes important.
-    if (!Parent) {
-      continue;
-    }
-    Parent = Parent->getDefinition();
-    for (CXXRecordDecl::method_iterator M = Parent->method_begin();
-         M != Parent->method_end(); ++M) {
-      if (hasCustomAnnotation(*M, "moz_must_override"))
-        MustOverrides.push_back(*M);
-    }
-  }
-
-  for (OverridesVector::iterator It = MustOverrides.begin();
-       It != MustOverrides.end(); ++It) {
-    bool Overridden = false;
-    for (CXXRecordDecl::method_iterator M = D->method_begin();
-         !Overridden && M != D->method_end(); ++M) {
-      // The way that Clang checks if a method M overrides its parent method
-      // is if the method has the same name but would not overload.
-      if (getNameChecked(M) == getNameChecked(*It) &&
-          !CI.getSema().IsOverload(*M, (*It), false)) {
-        Overridden = true;
-        break;
-      }
-    }
-    if (!Overridden) {
-      unsigned OverrideID = Diag.getDiagnosticIDs()->getCustomDiagID(
-          DiagnosticIDs::Error, "%0 must override %1");
-      unsigned OverrideNote = Diag.getDiagnosticIDs()->getCustomDiagID(
-          DiagnosticIDs::Note, "function to override is here");
-      Diag.Report(D->getLocation(), OverrideID) << D->getDeclName()
-                                                << (*It)->getDeclName();
-      Diag.Report((*It)->getLocation(), OverrideNote);
-    }
-  }
-
-  return true;
-}
-
 bool MozChecker::VisitSwitchCase(SwitchCase *Statement) {
   handleUnusedExprResult(Statement->getSubStmt());
   return true;

build/clang-plugin/MozChecker.h

@@ -9,11 +9,11 @@
 
 class MozChecker : public ASTConsumer, public RecursiveASTVisitor<MozChecker> {
   DiagnosticsEngine &Diag;
-  const CompilerInstance &CI;
   DiagnosticsMatcher Matcher;
 
 public:
-  MozChecker(const CompilerInstance &CI) : Diag(CI.getDiagnostics()), CI(CI) {}
+  MozChecker(CompilerInstance &CI) :
+    Diag(CI.getDiagnostics()), Matcher(CI) {}
   virtual ~MozChecker() {}
 
   ASTConsumerPtr getOtherConsumer() { return Matcher.makeASTConsumer(); }
@@ -24,8 +24,6 @@ public:
 
   void handleUnusedExprResult(const Stmt *Statement);
 
-  bool VisitCXXRecordDecl(CXXRecordDecl *D);
-
   bool VisitSwitchCase(SwitchCase *Statement);
   bool VisitCompoundStmt(CompoundStmt *Statement);
   bool VisitIfStmt(IfStmt *Statement);

build/clang-plugin/MustOverrideChecker.cpp

@@ -0,0 +1,62 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "MustOverrideChecker.h"
+#include "CustomMatchers.h"
+
+void MustOverrideChecker::registerMatchers(MatchFinder* AstMatcher) {
+  AstMatcher->addMatcher(cxxRecordDecl(isDefinition()).bind("class"), this);
+}
+
+void MustOverrideChecker::registerPPCallbacks(CompilerInstance& CI) {
+  this->CI = &CI;
+}
+
+void MustOverrideChecker::check(
+    const MatchFinder::MatchResult &Result) {
+  auto D = Result.Nodes.getNodeAs<CXXRecordDecl>("class");
+
+  // Look through all of our immediate bases to find methods that need to be
+  // overridden
+  typedef std::vector<CXXMethodDecl *> OverridesVector;
+  OverridesVector MustOverrides;
+  for (const auto& Base : D->bases()) {
+    // The base is either a class (CXXRecordDecl) or it's a templated class...
+    CXXRecordDecl *Parent = Base.getType()
+                                .getDesugaredType(D->getASTContext())
+                                ->getAsCXXRecordDecl();
+    // The parent might not be resolved to a type yet. In this case, we can't
+    // do any checking here. For complete correctness, we should visit
+    // template instantiations, but this case is likely to be rare, so we will
+    // ignore it until it becomes important.
+    if (!Parent) {
+      continue;
+    }
+    Parent = Parent->getDefinition();
+    for (const auto& M : Parent->methods()) {
+      if (MozChecker::hasCustomAnnotation(M, "moz_must_override"))
+        MustOverrides.push_back(M);
+    }
+  }
+
+  for (auto& O : MustOverrides) {
+    bool Overridden = false;
+    for (const auto& M : D->methods()) {
+      // The way that Clang checks if a method M overrides its parent method
+      // is if the method has the same name but would not overload.
+      if (getNameChecked(M) == getNameChecked(O) &&
+          !CI->getSema().IsOverload(M, O, false)) {
+        Overridden = true;
+        break;
+      }
+    }
+    if (!Overridden) {
+      diag(D->getLocation(), "%0 must override %1",
+           DiagnosticIDs::Error) << D->getDeclName()
+                                 << O->getDeclName();
+      diag(O->getLocation(), "function to override is here",
+           DiagnosticIDs::Note);
+    }
+  }
+}

build/clang-plugin/MustOverrideChecker.h

@@ -0,0 +1,23 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef MustOverrideChecker_h__
+#define MustOverrideChecker_h__
+
+#include "plugin.h"
+
+class MustOverrideChecker : public BaseCheck {
+public:
+  MustOverrideChecker(StringRef CheckName,
+                      ContextType *Context = nullptr)
+    : BaseCheck(CheckName, Context), CI(nullptr) {}
+  void registerMatchers(MatchFinder* AstMatcher) override;
+  void registerPPCallbacks(CompilerInstance& CI) override;
+  void check(const MatchFinder::MatchResult &Result) override;
+
+private:
+  const CompilerInstance* CI;
+};
+
+#endif

build/clang-plugin/moz.build

@@ -16,6 +16,7 @@ UNIFIED_SOURCES += [
     'KungFuDeathGripChecker.cpp',
     'MozCheckAction.cpp',
     'MozChecker.cpp',
+    'MustOverrideChecker.cpp',
     'NaNExprChecker.cpp',
     'NeedsNoVTableTypeChecker.cpp',
     'NoAddRefReleaseOnReturnChecker.cpp',