Extend Clang plugin to warn on missing OVERRIDE.

BUG=104314
TEST=none

Review URL: http://codereview.chromium.org/8586037

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111712 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 165 insertions, 28 deletions

diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
index 3499ca7..d191f52 100644
--- a/tools/clang/plugins/ChromeClassTester.cpp
+++ b/tools/clang/plugins/ChromeClassTester.cpp
@@ -43,6 +43,7 @@ ChromeClassTester::ChromeClassTester(CompilerInstance& instance)
 void ChromeClassTester::BuildBannedLists() {
   banned_namespaces_.push_back("std");
   banned_namespaces_.push_back("__gnu_cxx");
+  banned_namespaces_.push_back("WebKit");
 
   banned_directories_.push_back("third_party/");
   banned_directories_.push_back("native_client/");
@@ -153,11 +154,11 @@ void ChromeClassTester::emitWarning(SourceLocation loc, const char* raw_error) {
   DiagnosticBuilder B = diagnostic().Report(full, id);
 }
 
-bool ChromeClassTester::InTestingNamespace(Decl* record) {
+bool ChromeClassTester::InTestingNamespace(const Decl* record) {
   return GetNamespace(record).find("testing") != std::string::npos;
 }
 
-bool ChromeClassTester::InBannedNamespace(Decl* record) {
+bool ChromeClassTester::InBannedNamespace(const Decl* record) {
   std::string n = GetNamespace(record);
   if (n != "") {
     return std::find(banned_namespaces_.begin(), banned_namespaces_.end(), n)
@@ -167,7 +168,7 @@ bool ChromeClassTester::InBannedNamespace(Decl* record) {
   return false;
 }
 
-std::string ChromeClassTester::GetNamespace(Decl* record) {
+std::string ChromeClassTester::GetNamespace(const Decl* record) {
   return GetNamespaceImpl(record->getDeclContext(), "");
 }
 
diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h
index 81f3a50..5004f56 100644
--- a/tools/clang/plugins/ChromeClassTester.h
+++ b/tools/clang/plugins/ChromeClassTester.h
@@ -37,7 +37,11 @@ class ChromeClassTester : public clang::ASTConsumer {
   // Utility method for subclasses to check if testing details are in this
   // class. Some tests won't care if a class has a ::testing member and others
   // will.
-  bool InTestingNamespace(clang::Decl* record);
+  bool InTestingNamespace(const clang::Decl* record);
+
+  // Utility method for subclasses to check if this class is in a banned
+  // namespace.
+  bool InBannedNamespace(const clang::Decl* record);
 
  private:
   // Filtered versions of tags that are only called with things defined in
@@ -46,9 +50,8 @@ class ChromeClassTester : public clang::ASTConsumer {
                                 clang::CXXRecordDecl* record) = 0;
 
   // Utility methods used for filtering out non-chrome classes (and ones we
-  // delibrately ignore) in HandleTagDeclDefinition().
-  bool InBannedNamespace(clang::Decl* record);
-  std::string GetNamespace(clang::Decl* record);
+  // deliberately ignore) in HandleTagDeclDefinition().
+  std::string GetNamespace(const clang::Decl* record);
   std::string GetNamespaceImpl(const clang::DeclContext* context,
                                std::string candidate);
   bool InBannedDirectory(clang::SourceLocation loc);
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index 191e413..fbc5274 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -8,6 +8,7 @@
 // - Constructors/Destructors should not be inlined if they are of a complex
 //   class type.
 // - Missing "virtual" keywords on methods that should be virtual.
+// - Non-annotated overriding virtual methods.
 // - Virtual methods with nonempty implementations in their headers.
 //
 // Things that are still TODO:
@@ -38,8 +39,10 @@ bool TypeHasNonTrivialDtor(const Type* type) {
 // Searches for constructs that we know we don't want in the Chromium code base.
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
-  FindBadConstructsConsumer(CompilerInstance& instance)
-      : ChromeClassTester(instance) {}
+  FindBadConstructsConsumer(CompilerInstance& instance,
+                            bool skip_override)
+      : ChromeClassTester(instance),
+        skip_override_(skip_override) {}
 
   virtual void CheckChromeClass(const SourceLocation& record_location,
                                 CXXRecordDecl* record) {
@@ -47,7 +50,7 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     CheckVirtualMethods(record_location, record);
   }
 
-  // Prints errors if the constructor/destructor weight it too heavy.
+  // Prints errors if the constructor/destructor weight is too heavy.
   void CheckCtorDtorWeight(const SourceLocation& record_location,
                            CXXRecordDecl* record) {
     // We don't handle anonymous structs. If this record doesn't have a
@@ -136,16 +139,19 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     }
   }
 
-  void CheckVirtualMethod(const CXXMethodDecl *method) {
-    SourceLocation loc = method->getTypeSpecStartLoc();
-    if (isa<CXXDestructorDecl>(method))
-      loc = method->getInnerLocStart();
+  void CheckVirtualMethod(const CXXMethodDecl* method) {
+    if (!method->isVirtual())
+      return;
 
-    if (method->isVirtual() && !method->isVirtualAsWritten())
+    if (!method->isVirtualAsWritten()) {
+      SourceLocation loc = method->getTypeSpecStartLoc();
+      if (isa<CXXDestructorDecl>(method))
+        loc = method->getInnerLocStart();
       emitWarning(loc, "Overriding method must have \"virtual\" keyword.");
+    }
 
     // Virtual methods should not have inline definitions beyond "{}".
-    if (method->isVirtual() && method->hasBody() && method->hasInlineBody()) {
+    if (method->hasBody() && method->hasInlineBody()) {
       if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) {
         if (cs->size()) {
           emitWarning(
@@ -157,6 +163,37 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     }
   }
 
+  bool IsMethodInBannedNamespace(const CXXMethodDecl* method) {
+    if (InBannedNamespace(method))
+      return true;
+    for (CXXMethodDecl::method_iterator i = method->begin_overridden_methods();
+         i != method->end_overridden_methods();
+         ++i) {
+      const CXXMethodDecl* overridden = *i;
+      if (IsMethodInBannedNamespace(overridden))
+        return true;
+    }
+
+    return false;
+  }
+
+  void CheckOverriddenMethod(const CXXMethodDecl* method) {
+    if (skip_override_)
+      return;
+
+    if (!method->size_overridden_methods() || method->getAttr<OverrideAttr>())
+      return;
+
+    if (isa<CXXDestructorDecl>(method) || method->isPure())
+      return;
+
+    if (IsMethodInBannedNamespace(method))
+      return;
+
+    SourceLocation loc = method->getTypeSpecStartLoc();
+    emitWarning(loc, "Overriding method must be marked with OVERRIDE.");
+  }
+
   // Makes sure there is a "virtual" keyword on virtual methods and that there
   // are no inline function bodies on them (but "{}" is allowed).
   //
@@ -180,14 +217,14 @@ class FindBadConstructsConsumer : public ChromeClassTester {
 
     for (CXXRecordDecl::method_iterator it = record->method_begin();
          it != record->method_end(); ++it) {
-      if (it->isCopyAssignmentOperator() ||
-          dyn_cast<CXXConstructorDecl>(*it)) {
+      if (it->isCopyAssignmentOperator() || isa<CXXConstructorDecl>(*it)) {
         // Ignore constructors and assignment operators.
-      } else if (dyn_cast<CXXDestructorDecl>(*it) &&
+      } else if (isa<CXXDestructorDecl>(*it) &&
           !record->hasUserDeclaredDestructor()) {
-        // Ignore non-userdeclared destructors.
+        // Ignore non-user-declared destructors.
       } else {
         CheckVirtualMethod(*it);
+        CheckOverriddenMethod(*it);
       }
     }
   }
@@ -233,7 +270,7 @@ class FindBadConstructsConsumer : public ChromeClassTester {
         break;
       }
       case Type::Typedef: {
-        while (const TypedefType *TT = dyn_cast<TypedefType>(type)) {
+        while (const TypedefType* TT = dyn_cast<TypedefType>(type)) {
           type = TT->getDecl()->getUnderlyingType().getTypePtr();
         }
         CountType(type, trivial_member, non_trivial_member,
@@ -248,19 +285,41 @@ class FindBadConstructsConsumer : public ChromeClassTester {
       }
     }
   }
+
+ private:
+  // TODO(avi): Remove this (and all related code) once the override warning is
+  // enabled on all bots.
+  bool skip_override_;
 };
 
 class FindBadConstructsAction : public PluginASTAction {
+ public:
+  FindBadConstructsAction() : skip_override_(false) {
+  }
+
  protected:
   ASTConsumer* CreateASTConsumer(CompilerInstance &CI, llvm::StringRef ref) {
-    return new FindBadConstructsConsumer(CI);
+    return new FindBadConstructsConsumer(CI, skip_override_);
   }
 
   bool ParseArgs(const CompilerInstance &CI,
                  const std::vector<std::string>& args) {
-    // We don't take any additional arguments here.
-    return true;
+    bool parsed = true;
+
+    for (size_t i = 0; i < args.size() && parsed; ++i) {
+      if (args[i] == "skip-override") {
+        skip_override_ = true;
+      } else {
+        parsed = false;
+        llvm::errs() << "Unknown arg: " << args[i] << "\n";
+      }
+    }
+
+    return parsed;
   }
+
+ private:
+  bool skip_override_;
 };
 
 }  // namespace
diff --git a/tools/clang/plugins/tests/overridden_methods.cpp b/tools/clang/plugins/tests/overridden_methods.cpp
new file mode 100644
index 0000000..f81d3a2
--- /dev/null
+++ b/tools/clang/plugins/tests/overridden_methods.cpp
@@ -0,0 +1,14 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "overridden_methods.h"
+
+// Fill in the implementations
+void DerivedClass::SomeMethod() {}
+void DerivedClass::SomeOtherMethod() {}
+void DerivedClass::WebKitModifiedSomething() {}
+
+int main() {
+  DerivedClass something;
+}
diff --git a/tools/clang/plugins/tests/overridden_methods.h b/tools/clang/plugins/tests/overridden_methods.h
new file mode 100644
index 0000000..08ef0a5
--- /dev/null
+++ b/tools/clang/plugins/tests/overridden_methods.h
@@ -0,0 +1,51 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef OVERRIDDEN_METHODS_H_
+#define OVERRIDDEN_METHODS_H_
+
+// Should warn about overriding of methods.
+class BaseClass {
+ public:
+  virtual ~BaseClass() {}
+  virtual void SomeMethod() = 0;
+  virtual void SomeOtherMethod() = 0;
+  virtual void SomeInlineMethod() = 0;
+};
+
+class InterimClass : public BaseClass {
+  // Should not warn about pure virtual methods.
+  virtual void SomeMethod() = 0;
+};
+
+namespace WebKit {
+class WebKitObserver {
+ public:
+  virtual void WebKitModifiedSomething() {};
+};
+}  // namespace WebKit
+
+namespace webkit_glue {
+class WebKitObserverImpl : WebKit::WebKitObserver {
+ public:
+  virtual void WebKitModifiedSomething() {};
+};
+}  // namespace webkit_glue
+
+class DerivedClass : public InterimClass,
+                     public webkit_glue::WebKitObserverImpl {
+ public:
+  // Should not warn about destructors.
+  virtual ~DerivedClass() {}
+  // Should warn.
+  virtual void SomeMethod();
+  // Should not warn if marked as override.
+  virtual void SomeOtherMethod() override;
+  // Should warn for inline implementations.
+  virtual void SomeInlineMethod() {}
+  // Should not warn if overriding a method whose origin is WebKit.
+  virtual void WebKitModifiedSomething();
+};
+
+#endif  // OVERRIDDEN_METHODS_H_
diff --git a/tools/clang/plugins/tests/overridden_methods.txt b/tools/clang/plugins/tests/overridden_methods.txt
new file mode 100644
index 0000000..da3c58f
--- /dev/null
+++ b/tools/clang/plugins/tests/overridden_methods.txt
@@ -0,0 +1,8 @@
+In file included from overridden_methods.cpp:5:
+./overridden_methods.h:42:11: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+  virtual void SomeMethod();
+          ^
+./overridden_methods.h:46:11: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+  virtual void SomeInlineMethod() {}
+          ^
+2 warnings generated.
diff --git a/tools/clang/plugins/tests/test.sh b/tools/clang/plugins/tests/test.sh
index fcfd213..262ebbb 100755
--- a/tools/clang/plugins/tests/test.sh
+++ b/tools/clang/plugins/tests/test.sh
@@ -23,7 +23,7 @@ usage() {
 
 # Runs a single test case.
 do_testcase() {
-  local output="$("${CLANG_DIR}"/bin/clang -c \
+  local output="$("${CLANG_DIR}"/bin/clang -c -Wno-c++11-extensions \
       -Xclang -load -Xclang "${CLANG_DIR}"/lib/libFindBadConstructs.${LIB} \
       -Xclang -plugin -Xclang find-bad-constructs ${1} 2>&1)"
   local diffout="$(echo "${output}" | diff - "${2}")"
diff --git a/tools/clang/plugins/tests/virtual_methods.h b/tools/clang/plugins/tests/virtual_methods.h
index d256f07..d9fbf96 100644
--- a/tools/clang/plugins/tests/virtual_methods.h
+++ b/tools/clang/plugins/tests/virtual_methods.h
@@ -20,7 +20,7 @@ class VirtualMethodsInHeaders {
 // Complain on missing 'virtual' keyword in overrides.
 class WarnOnMissingVirtual : public VirtualMethodsInHeaders {
  public:
-  void MethodHasNoArguments();
+  void MethodHasNoArguments() override;
 };
 
 // Don't complain about things in a 'testing' namespace.
diff --git a/tools/clang/plugins/tests/virtual_methods.txt b/tools/clang/plugins/tests/virtual_methods.txt
index 5eb6657..571d6d6 100644
--- a/tools/clang/plugins/tests/virtual_methods.txt
+++ b/tools/clang/plugins/tests/virtual_methods.txt
@@ -3,6 +3,6 @@ In file included from virtual_methods.cpp:5:
   virtual bool ComplainAboutThis() { return true; }
                                    ^
 ./virtual_methods.h:23:3: warning: [chromium-style] Overriding method must have "virtual" keyword.
-  void MethodHasNoArguments();
+  void MethodHasNoArguments() override;
   ^
 2 warnings generated.
diff --git a/tools/clang/scripts/plugin_flags.sh b/tools/clang/scripts/plugin_flags.sh
index ec3eed6c..c4deb9c 100755
--- a/tools/clang/scripts/plugin_flags.sh
+++ b/tools/clang/scripts/plugin_flags.sh
@@ -17,4 +17,5 @@ else
 fi
 
 echo -Xclang -load -Xclang $CLANG_LIB_PATH/libFindBadConstructs.$LIBSUFFIX \
-  -Xclang -add-plugin -Xclang find-bad-constructs
+  -Xclang -add-plugin -Xclang find-bad-constructs \
+  -Xclang -plugin-arg-find-bad-constructs -Xclang skip-override