Check for public dtors on base::RefCounted types

tools/clang/plugins/FindBadConstructs.cpp

Index: tools/clang/plugins/FindBadConstructs.cpp
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index 4b2f8009dcf64acb5380d9ff8706148e8b98f859..da0e9e3edcf9ca71cd6e420540b3da33011ccf78 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -18,6 +18,7 @@
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/AST.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -36,6 +37,28 @@ bool TypeHasNonTrivialDtor(const Type* type) {
   return false;
 }
 
+// Returns true if |base| specifies one of the Chromium reference counted
+// classes (base::RefCounted / base::RefCountedThreadSafe). |user_data| is
+// ignored.
+bool IsRefCounted(const CXXBaseSpecifier* base,
+                  CXXBasePath& path,
+                  void* user_data) {
+  if (base->getTypeSourceInfo()->getTypeLoc().getTypeLocClass() !=
+      TypeLoc::TemplateSpecialization) {
+    return false;
+  }
+
+  TemplateName name =
+      dyn_cast<TemplateSpecializationType>(base->getType())->getTemplateName();
+
+  if (TemplateDecl* decl = name.getAsTemplateDecl()) {
+    std::string base_name = decl->getNameAsString();

[Nico, 2012/04/05 23:08:13]

might want to check the namespace too

+    if (base_name.compare(0, 10, "RefCounted") == 0)
+      return true;
+  }
+  return false;
+}
+
 // Searches for constructs that we know we don't want in the Chromium code base.
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
@@ -44,8 +67,46 @@ class FindBadConstructsConsumer : public ChromeClassTester {
 
   virtual void CheckChromeClass(const SourceLocation& record_location,
                                 CXXRecordDecl* record) {
-    CheckCtorDtorWeight(record_location, record);
-    CheckVirtualMethods(record_location, record);
+    bool implementation_file = InImplementationFile(record_location);
+
+    if (!implementation_file) {
+      // Only check for "heavy" constructors/destructors in header files;
+      // within implementation files, there is no performance cost.
+      CheckCtorDtorWeight(record_location, record);
+    }
+
+    CheckRefCountedDtors(record_location, record);
+
+    // Check that all virtual methods are marked accordingly with both
+    // virtual and OVERRIDE. However, only warn on inline bodies for
+    // declarations in headers, not implementation files.
+    CheckVirtualMethods(record_location, record, !implementation_file);
+  }
+
+  // Prints errors if the destructor of a RefCounted class is public.
+  void CheckRefCountedDtors(const SourceLocation& record_location,
+                            CXXRecordDecl* record) {
+    // Skip anonymous structs.
+    if (record->getIdentifier() == NULL)
+      return;
+
+    CXXBasePaths paths;
+    if (!record->lookupInBases(&IsRefCounted, NULL, paths))
+      return;  // Class does not derive from a ref-counted base class.
+
+    if (!record->hasUserDeclaredDestructor()) {
+      emitWarning(
+          record_location,
+          "Classes that are ref-counted should have explicit "
+          "destructors that are protected or private.");
+    } else if (CXXDestructorDecl* dtor = record->getDestructor()) {
+      if (dtor->getAccess() == AS_public) {
+        emitWarning(
+            dtor->getInnerLocStart(),
+            "Classes that are ref-counted should not have "
+            "public destructors.");
+      }
+    }
   }
 
   // Prints errors if the constructor/destructor weight is too heavy.
@@ -137,7 +198,8 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     }
   }
 
-  void CheckVirtualMethod(const CXXMethodDecl* method) {
+  void CheckVirtualMethod(const CXXMethodDecl* method,
+                          bool warn_on_inline_bodies) {
     if (!method->isVirtual())
       return;
 
@@ -148,8 +210,10 @@ class FindBadConstructsConsumer : public ChromeClassTester {
       emitWarning(loc, "Overriding method must have \"virtual\" keyword.");
     }
 
-    // Virtual methods should not have inline definitions beyond "{}".
-    if (method->hasBody() && method->hasInlineBody()) {
+    // Virtual methods should not have inline definitions beyond "{}". This
+    // only matters for header files.
+    if (warn_on_inline_bodies && method->hasBody() &&
+        method->hasInlineBody()) {
       if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) {
         if (cs->size()) {
           emitWarning(
@@ -189,15 +253,15 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     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).
+  // Makes sure there is a "virtual" keyword on virtual methods.
   //
   // 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) {
+                           CXXRecordDecl* record,
+                           bool warn_on_inline_bodies) {
     for (CXXRecordDecl::field_iterator it = record->field_begin();
          it != record->field_end(); ++it) {
       CXXRecordDecl* record_type =
@@ -218,7 +282,7 @@ class FindBadConstructsConsumer : public ChromeClassTester {
           !record->hasUserDeclaredDestructor()) {
         // Ignore non-user-declared destructors.
       } else {
-        CheckVirtualMethod(*it);
+        CheckVirtualMethod(*it, warn_on_inline_bodies);
         CheckOverriddenMethod(*it);
       }
     }