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

tools/clang/plugins/FindBadConstructsConsumer.cpp

 // Copyright (c) 2012 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 "FindBadConstructsConsumer.h"
 
 #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;
 
 namespace chrome_checker {
 
 namespace {
 
 const char kMethodRequiresOverride[] =
     "[chromium-style] Overriding method must be marked with 'override' or "
@@ skipping 71 matching lines @@
   return FixItHint::CreateRemoval(range);
 }
 
 bool IsPodOrTemplateType(const CXXRecordDecl& record) {
   return record.isPOD() ||
          record.getDescribedClassTemplate() ||
          record.getTemplateSpecializationKind() ||
          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,
                                                      const Options& options)
     : ChromeClassTester(instance, options) {
   // Messages for virtual method specifiers.
   diag_method_requires_override_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresOverride);
   diag_redundant_virtual_specifier_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kRedundantVirtualSpecifier);
@@ skipping 47 matching lines @@
     // Only check for "heavy" constructors/destructors in header files;
     // within implementation files, there is no performance cost.
 
     // If this is a POD or a class template or a type dependent on a
     // templated class, assume there's no ctor/dtor/virtual method
     // optimization that we should do.
     if (!IsPodOrTemplateType(*record))
       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);
 
   CheckWeakPtrFactoryMembers(record_location, record);
 }
 
 void FindBadConstructsConsumer::CheckChromeEnum(SourceLocation enum_location,
                                                 EnumDecl* enum_decl) {
   if (!options_.check_enum_last_value)
     return;
@@ skipping 81 matching lines @@
   dtor_score += templated_base_classes * 9;
   // Instantiating a template is an insta-hit.
   dtor_score += templated_non_trivial_member * 10;
   // The fourth normal class member should trigger the warning.
   dtor_score += non_trivial_member * 3;
 
   int ctor_score = dtor_score;
   // 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,
                   "Complex class/struct needs an explicit out-of-line "
                   "constructor.");
     } else {
       // Iterate across all the constructors in this file and yell if we
       // find one that tries to be inline.
       for (CXXRecordDecl::ctor_iterator it = record->ctor_begin();
            it != record->ctor_end();
@@ skipping 88 matching lines @@
 
 SuppressibleDiagnosticBuilder
 FindBadConstructsConsumer::ReportIfSpellingLocNotIgnored(
     SourceLocation loc,
     unsigned diagnostic_id) {
   return SuppressibleDiagnosticBuilder(
       &diagnostic(), loc, diagnostic_id,
       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),
   // there's a really high chance we won't care about these errors
   for (CXXRecordDecl::field_iterator it = record->field_begin();
        it != record->field_end();
        ++it) {
     CXXRecordDecl* record_type = it->getTypeSourceInfo()
                                      ->getTypeLoc()
                                      .getTypePtr()
@@ skipping 10 matching lines @@
        ++it) {
     if (it->isCopyAssignmentOperator() || isa<CXXConstructorDecl>(*it)) {
       // Ignore constructors and assignment operators.
     } else if (isa<CXXDestructorDecl>(*it) &&
                !record->hasUserDeclaredDestructor()) {
       // Ignore non-user-declared destructors.
     } else if (!it->isVirtual()) {
       continue;
     } else {
       CheckVirtualSpecifiers(*it);
-      if (warn_on_inline_bodies)
-        CheckVirtualBodies(*it);
     }
   }
 }
 
 // Makes sure that virtual methods use the most appropriate specifier. If a
 // virtual method overrides a method from a base class, only the override
 // specifier should be used. If the method should not be overridden by derived
 // classes, only the final specifier should be used.
 void FindBadConstructsConsumer::CheckVirtualSpecifiers(
     const CXXMethodDecl* method) {
@@ skipping 81 matching lines @@
   }
 
   if (final_attr && !is_override) {
     ReportIfSpellingLocNotIgnored(method->getLocStart(),
                                   diag_base_method_virtual_and_final_)
         << FixItRemovalForVirtual(manager, lang_opts, method)
         << FixItHint::CreateRemoval(final_attr->getRange());
   }
 }
 
-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,
                                           int* templated_non_trivial_member) {
   switch (type->getTypeClass()) {
     case Type::Record: {
       // Simplifying; the whole class isn't trivial if the dtor is, but
       // we use this as a signal about complexity.
       if (TypeHasNonTrivialDtor(type))
         (*non_trivial_member)++;
@@ skipping 299 matching lines @@
     // one of those, it means there is at least one member after a factory.
     if (weak_ptr_factory_location.isValid() &&
         !param_is_weak_ptr_factory_to_self) {
       diagnostic().Report(weak_ptr_factory_location,
                           diag_weak_ptr_factory_order_);
     }
   }
 }
 
 }  // namespace chrome_checker