Convert the no-inline-virtuals rule into a constructors rule.

tools/clang/plugins/FindBadConstructsConsumer.cpp

Index: tools/clang/plugins/FindBadConstructsConsumer.cpp
diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp
index c79a764eb953be319cb5e83fe766cee2bbaa8a3c..de31588b6815427b6c8f639203278ee2ade864e5 100644
--- a/tools/clang/plugins/FindBadConstructsConsumer.cpp
+++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp
@@ -7,6 +7,7 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/AST/Attr.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
@@ -98,6 +99,44 @@ bool IsPodOrTemplateType(const CXXRecordDecl& record) {
          record.isDependentType();
 }
 
+// True if this class is likely to be created by user code rather than derived
+// from.
+bool IsLeafClass(const CXXRecordDecl& record) {

[dcheng, 2016/03/24 17:37:25]

How accurate is this heuristic for determining leafiness? What were the results
when you built Chromium with your plugin changes active?

+  if (record.isAbstract())
+    return false;
+  bool found_protected = false;
+  for (const CXXConstructorDecl* ctor : record.ctors()) {
+    if (ctor->isDeleted()) {
+      continue;
+    }
+    switch (ctor->getAccess()) {
+      case AS_public:
+        return true;
+      case AS_protected:
+        found_protected = true;
+        break;
+      case AS_private:
+        break;
+      case AS_none:
+        // Shouldn't happen.
+        break;
+    }
+  }
+  // If all constructors are private, assume there's a public Create() function.
+  return !found_protected;
+}
+
+// Returns the distance from r.begin() to r.end(), but stops if it reaches
+// |at_most|.
+template<typename Range>
+int distance_at_most(const Range& r, int at_most) {
+  int result = 0;
+  for (auto it = r.begin(), e = r.end(); it != e && result < at_most; ++it) {
+    ++result;
+  }
+  return result;
+}
+
 }  // namespace
 
 FindBadConstructsConsumer::FindBadConstructsConsumer(CompilerInstance& instance,
@@ -165,14 +204,13 @@ void FindBadConstructsConsumer::CheckChromeClass(SourceLocation record_location,
       CheckCtorDtorWeight(record_location, record);
   }
 
-  bool warn_on_inline_bodies = !implementation_file;
   // Check that all virtual methods are annotated with override or final.
   // Note this could also apply to templates, but for some reason Clang
   // does not always see the "override", so we get false positives.
   // See http://llvm.org/bugs/show_bug.cgi?id=18440 and
   //     http://llvm.org/bugs/show_bug.cgi?id=21942
   if (!IsPodOrTemplateType(*record))
-    CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
+    CheckVirtualMethods(record_location, record);
 
   CheckRefCountedDtors(record_location, record);
 
@@ -274,6 +312,43 @@ void FindBadConstructsConsumer::CheckCtorDtorWeight(
   // You should be able to have 9 ints before we warn you.
   ctor_score += trivial_member;
 
+  // For leaf classes, count the cost of emitting the vtable on Windows.
+  // (Windows because MSVC doesn't use the key-function optimization, so vtables
+  // are emitted more often.) The data behind this is in
+  // https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/discussion
+  if (options_.treat_virtuals_as_complexity && record->isPolymorphic() &&
+      IsLeafClass(*record)) {
+    int virtual_score = 3;
+
+    // For each non-overridden virtual function, add the length of its inline
+    // body, if any.
+    CXXFinalOverriderMap final_overriders;
+    llvm::SmallPtrSet<const Stmt*, 10> overriding_bodies;
+    record->getFinalOverriders(final_overriders);
+    for (const std::pair<const CXXMethodDecl*, OverridingMethods>& method :

[dcheng, 2016/03/24 17:37:25]

Another way to write this might be to iterate over CXXRecordDecl::methods():
would doing that instead work here? I don't think we'd need the deduping set in
that case, and we can use CXXMethodDecl::size_overridden_methods() to determine
if it's an override or not.

+         final_overriders) {
+      for (const auto& overrides : method.second) {
+        for (const UniqueVirtualMethod& override : overrides.second) {
+          if (override.Method->hasInlineBody()) {
+            if (const Stmt* body = override.Method->getBody()) {
+              // The same inline function can override multiple virtual
+              // functions in base classes, and we only want to count it once.
+              overriding_bodies.insert(body);
+            }
+          }
+        }
+      }
+    }
+    for (const Stmt* body : overriding_bodies) {
+      virtual_score += 1 + distance_at_most(body->children(), 10);
+    }
+
+    ctor_score += virtual_score;
+    if (!record->getDestructor()->isVirtual()) {
+      dtor_score += virtual_score;
+    }
+  }
+
   if (ctor_score >= 10) {
     if (!record->hasUserDeclaredConstructor()) {
       emitWarning(record_location,
@@ -382,12 +457,10 @@ FindBadConstructsConsumer::ReportIfSpellingLocNotIgnored(
       InBannedDirectory(instance().getSourceManager().getSpellingLoc(loc)));
 }
 
-// Checks that virtual methods are correctly annotated, and have no body in a
-// header file.
+// Checks that virtual methods are correctly annotated.
 void FindBadConstructsConsumer::CheckVirtualMethods(
     SourceLocation record_location,
-    CXXRecordDecl* record,
-    bool warn_on_inline_bodies) {
+    CXXRecordDecl* record) {
   // 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),
@@ -418,8 +491,6 @@ void FindBadConstructsConsumer::CheckVirtualMethods(
       continue;
     } else {
       CheckVirtualSpecifiers(*it);
-      if (warn_on_inline_bodies)
-        CheckVirtualBodies(*it);
     }
   }
 }
@@ -521,39 +592,6 @@ void FindBadConstructsConsumer::CheckVirtualSpecifiers(
   }
 }
 
-void FindBadConstructsConsumer::CheckVirtualBodies(
-    const CXXMethodDecl* method) {
-  // Virtual methods should not have inline definitions beyond "{}". This
-  // only matters for header files.
-  if (method->hasBody() && method->hasInlineBody()) {
-    if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) {
-      if (cs->size()) {
-        SourceLocation loc = cs->getLBracLoc();
-        // CR_BEGIN_MSG_MAP_EX and BEGIN_SAFE_MSG_MAP_EX try to be compatible
-        // to BEGIN_MSG_MAP(_EX).  So even though they are in chrome code,
-        // we can't easily fix them, so explicitly whitelist them here.
-        bool emit = true;
-        if (loc.isMacroID()) {
-          SourceManager& manager = instance().getSourceManager();
-          if (InBannedDirectory(manager.getSpellingLoc(loc)))
-            emit = false;
-          else {
-            StringRef name = Lexer::getImmediateMacroName(
-                loc, manager, instance().getLangOpts());
-            if (name == "CR_BEGIN_MSG_MAP_EX" ||
-                name == "BEGIN_SAFE_MSG_MAP_EX")
-              emit = false;
-          }
-        }
-        if (emit)
-          emitWarning(loc,
-                      "virtual methods with non-empty bodies shouldn't be "
-                      "declared inline.");
-      }
-    }
-  }
-}
-
 void FindBadConstructsConsumer::CountType(const Type* type,
                                           int* trivial_member,
                                           int* non_trivial_member,