Change how we detect test code so we bail on virtual checks when we see gmock internals.

(There are maybe ten more things to cleanup in the test code itself after this
patch, but I want to get to the path handling problems.)

Also, per popular request, s/chrome-style/chromium-style/; in the error messages.

BUG=none

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

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

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() ||