3 files changed, 44 insertions, 27 deletions

diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
index 106f16e..d98a44c 100644
--- a/tools/clang/plugins/ChromeClassTester.cpp
+++ b/tools/clang/plugins/ChromeClassTester.cpp
@@ -7,8 +7,6 @@
 
 #include "ChromeClassTester.h"
 
-#include "clang/Basic/FileManager.h"
-
 using namespace clang;
 
 namespace {
@@ -18,6 +16,9 @@ bool starts_with(const std::string& one, const std::string& two) {
 }
 
 bool ends_with(const std::string& one, const std::string& two) {
+  if (two.size() > one.size())
+    return false;
+
   return one.substr(one.size() - two.size(), two.size()) == two;
 }
 
@@ -59,6 +60,13 @@ ChromeClassTester::ChromeClassTester(CompilerInstance& instance)
   ignored_record_names_.push_back("AutoTaskRunner");
   ignored_record_names_.push_back("AutoCallbackRunner");
 
+  // Has a UNIT_TEST only constructor. Isn't *terribly* complex...
+  ignored_record_names_.push_back("AutocompleteController");
+  ignored_record_names_.push_back("HistoryURLProvider");
+
+  // Because of chrome frame
+  ignored_record_names_.push_back("ReliabilityTestSuite");
+
   // Part of our public interface that nacl and friends use. (Arguably, this
   // should mean that this is a higher priority but fixing this looks hard.)
   ignored_record_names_.push_back("PluginVersionInfo");
@@ -84,23 +92,18 @@ void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) {
     if (InBannedDirectory(record_location))
       return;
 
-    // For now, due to large amounts of inlining in our unit test code, there's
-    // no way in hell we'll pass our checks but we want to deploy clang plugins
-    // now.
-    //
-    // TODO(erg): Deinline all our test code, and only perform something like
-    // this check for the "virtual" inlining tests on GMOCK code (since all of
-    // that is autogenerated and doesn't specify the "virtual" keyword).
-    if (IsTestCode(record)) {
-      return;
-    }
-
     // We sadly need to maintain a blacklist of types that violate these
     // rules, but do so for good reason or due to limitations of this
     // checker (i.e., we don't handle extern templates very well).
-    if (IsIgnoredType(record))
+    std::string base_name = record->getNameAsString();
+    if (IsIgnoredType(base_name))
       return;
 
+    // We ignore all classes that end with "Matcher" because they're probably
+    // GMock artifacts.
+    if (ends_with(base_name, "Matcher"))
+        return;
+
     CheckChromeClass(record_location, record);
   }
 }
@@ -108,20 +111,14 @@ void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) {
 void ChromeClassTester::emitWarning(SourceLocation loc, const char* raw_error) {
   FullSourceLoc full(loc, instance().getSourceManager());
   std::string err;
-  err = "[chrome-style] ";
+  err = "[chromium-style] ";
   err += raw_error;
   unsigned id = diagnostic().getCustomDiagID(Diagnostic::Warning, err);
   DiagnosticBuilder B = diagnostic().Report(full, id);
 }
 
-bool ChromeClassTester::IsTestCode(Decl* record) {
-  if (instance_.hasSourceManager()) {
-    SourceManager& m = instance_.getSourceManager();
-    std::string name = m.getFileEntryForID(m.getMainFileID())->getName();
-    return name.find("test") != name.npos ||
-        name.find("mock") != name.npos;
-  }
-  return false;
+bool ChromeClassTester::InTestingNamespace(Decl* record) {
+  return GetNamespace(record).find("testing") != std::string::npos;
 }
 
 bool ChromeClassTester::InBannedNamespace(Decl* record) {
@@ -195,8 +192,7 @@ bool ChromeClassTester::InBannedDirectory(const SourceLocation& loc) {
   return false;
 }
 
-bool ChromeClassTester::IsIgnoredType(RecordDecl* record) {
-  std::string base_name = record->getNameAsString();
+bool ChromeClassTester::IsIgnoredType(const std::string& base_name) {
   for (std::vector<std::string>::const_iterator it =
            ignored_record_names_.begin();
        it != ignored_record_names_.end(); ++it) {
diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h
index b9985d7..38b15e1 100644
--- a/tools/clang/plugins/ChromeClassTester.h
+++ b/tools/clang/plugins/ChromeClassTester.h
@@ -31,6 +31,11 @@ class ChromeClassTester : public clang::ASTConsumer {
   // printing.
   void emitWarning(clang::SourceLocation loc, const char* error);
 
+  // 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);
+
  private:
   // Template method which is called with only classes that are defined in
   // chrome header files.
@@ -39,13 +44,12 @@ class ChromeClassTester : public clang::ASTConsumer {
 
   // Utility methods used for filtering out non-chrome classes (and ones we
   // delibrately ignore) in HandleTagDeclDefinition().
-  bool IsTestCode(clang::Decl* record);
   bool InBannedNamespace(clang::Decl* record);
   std::string GetNamespace(clang::Decl* record);
   std::string GetNamespaceImpl(const clang::DeclContext* context,
                                std::string candidate);
   bool InBannedDirectory(const clang::SourceLocation& loc);
-  bool IsIgnoredType(clang::RecordDecl* record);
+  bool IsIgnoredType(const std::string& base_name);
 
   clang::CompilerInstance& instance_;
   clang::Diagnostic& diagnostic_;
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index 60b44dba..d7bcdae 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -135,8 +135,25 @@ class FindBadConstructsConsumer : public ChromeClassTester {
 
   // Makes sure there is a "virtual" keyword on virtual methods and that there
   // are no inline function bodies on them (but "{}" is allowed).
+  //
+  // Gmock objects trigger these for each MOCK_BLAH() macro used. So we have a
+  // trick to get around that. If a class has member variables whose types are
+  // in the "testing" namespace (which is how gmock works behind the scenes),
+  // there's a really high chance we won't care about these errors
   void CheckVirtualMethods(const SourceLocation& record_location,
                            CXXRecordDecl* record) {
+    for (CXXRecordDecl::field_iterator it = record->field_begin();
+         it != record->field_end(); ++it) {
+      CXXRecordDecl* record_type =
+          it->getTypeSourceInfo()->getTypeLoc().getTypePtr()->
+          getAsCXXRecordDecl();
+      if (record_type) {
+        if (InTestingNamespace(record_type)) {
+          return;
+        }
+      }
+    }
+
     for (CXXRecordDecl::method_iterator it = record->method_begin();
          it != record->method_end(); ++it) {
       if (it->isCopyAssignmentOperator() ||