Check implementation files for virtual and OVERRIDE

tools/clang/plugins/FindBadConstructs.cpp

 // Copyright (c) 2011 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.
 
 // This file defines a bunch of recurring problems in the Chromium C++ code.
 //
 // Checks that are implemented:
 // - Constructors/Destructors should not be inlined if they are of a complex
 //   class type.
 // - Missing "virtual" keywords on methods that should be virtual.
@@ skipping 31 matching lines @@
     return UnwrapType(elaborated->getNamedType().getTypePtr());
   if (const TypedefType* typedefed = dyn_cast<TypedefType>(type))
     return UnwrapType(typedefed->desugar().getTypePtr());
   return type;
 }
 
 // Searches for constructs that we know we don't want in the Chromium code base.
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
   FindBadConstructsConsumer(CompilerInstance& instance,
-                            bool check_refcounted_dtors)
+                            bool check_refcounted_dtors,
+                            bool check_virtuals_in_implementations)
       : ChromeClassTester(instance),
-        check_refcounted_dtors_(check_refcounted_dtors) {
+        check_refcounted_dtors_(check_refcounted_dtors),
+        check_virtuals_in_implementations_(check_virtuals_in_implementations) {
   }
 
   virtual void CheckChromeClass(SourceLocation record_location,
                                 CXXRecordDecl* 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);
-
-      // Check that all virtual methods are marked accordingly with both
-      // virtual and OVERRIDE.
-      CheckVirtualMethods(record_location, record);
     }
 
-    if (check_refcounted_dtors_)
+    if (!implementation_file || check_virtuals_in_implementations_) {
+      // 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);

[Nico, 2012/04/17 00:46:01]

Do:

bool warn_on_inline_bodies = !implementation_file;

pass that as parameter, and revert the change to the comment.

+    }
+
+    if (check_refcounted_dtors_) {
+      // Check to make sure that a class that derives from base::RefCounted
+      // or base::RefCountedThreadSafe does not have a public destructor.

[Nico, 2012/04/17 00:46:01]

useless comment, remove (the method comment already says this; if you feel it's
worth repeating give the method a better name instead)

       CheckRefCountedDtors(record_location, record);
+    }
   }
 
  private:
   bool check_refcounted_dtors_;
+  bool check_virtuals_in_implementations_;
 
   // Returns true if |base| specifies one of the Chromium reference counted
   // classes (base::RefCounted / base::RefCountedThreadSafe). |user_data| is
   // ignored.
   static bool IsRefCountedCallback(const CXXBaseSpecifier* base,
                                    CXXBasePath& path,
                                    void* user_data) {
     FindBadConstructsConsumer* self =
         static_cast<FindBadConstructsConsumer*>(user_data);
 
@@ skipping 131 matching lines @@
             "destructor.");
       } else if (CXXDestructorDecl* dtor = record->getDestructor()) {
         if (dtor->hasInlineBody()) {
           emitWarning(dtor->getInnerLocStart(),
                       "Complex destructor has an inline body.");
         }
       }
     }
   }
 
-  void CheckVirtualMethod(const CXXMethodDecl* method) {
+  void CheckVirtualMethod(const CXXMethodDecl* method,
+                          bool warn_on_inline_bodies) {
     if (!method->isVirtual())
       return;
 
     if (!method->isVirtualAsWritten()) {
       SourceLocation loc = method->getTypeSpecStartLoc();
       if (isa<CXXDestructorDecl>(method))
         loc = method->getInnerLocStart();
       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(
               cs->getLBracLoc(),
               "virtual methods with non-empty bodies shouldn't be "
               "declared inline.");
         }
       }
     }
   }
@@ skipping 23 matching lines @@
     if (isa<CXXDestructorDecl>(method) || method->isPure())
       return;
 
     if (IsMethodInBannedNamespace(method))
       return;
 
     SourceLocation loc = method->getTypeSpecStartLoc();
     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(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 =
           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() || isa<CXXConstructorDecl>(*it)) {
         // Ignore constructors and assignment operators.
       } else if (isa<CXXDestructorDecl>(*it) &&
           !record->hasUserDeclaredDestructor()) {
         // Ignore non-user-declared destructors.
       } else {
-        CheckVirtualMethod(*it);
+        CheckVirtualMethod(*it, warn_on_inline_bodies);
         CheckOverriddenMethod(*it);
       }
     }
   }
 
   void CountType(const Type* type,
                  int* trivial_member,
                  int* non_trivial_member,
                  int* templated_non_trivial_member) {
     switch (type->getTypeClass()) {
@@ skipping 45 matching lines @@
         // the 20 integer types.
         (*trivial_member)++;
         break;
       }
     }
   }
 };
 
 class FindBadConstructsAction : public PluginASTAction {
  public:
-  FindBadConstructsAction() : check_refcounted_dtors_(true) {}
+  FindBadConstructsAction()
+      : check_refcounted_dtors_(true),
+        check_virtuals_in_implementations_(true) {
+  }
 
  protected:
   ASTConsumer* CreateASTConsumer(CompilerInstance &CI, llvm::StringRef ref) {
-    return new FindBadConstructsConsumer(CI, check_refcounted_dtors_);
+    return new FindBadConstructsConsumer(
+        CI, check_refcounted_dtors_, check_virtuals_in_implementations_);
   }
 
   bool ParseArgs(const CompilerInstance &CI,
                  const std::vector<std::string>& args) {
     bool parsed = true;
 
     for (size_t i = 0; i < args.size() && parsed; ++i) {
       if (args[i] == "skip-refcounted-dtors") {
         check_refcounted_dtors_ = false;
+      } else if (args[i] == "skip-virtuals-in-implementations") {
+        check_virtuals_in_implementations_ = false;
       } else {
         parsed = false;
         llvm::errs() << "Unknown argument: " << args[i] << "\n";
       }
     }
 
     return parsed;
   }
 
  private:
   bool check_refcounted_dtors_;
+  bool check_virtuals_in_implementations_;
 };
 
 }  // namespace
 
 static FrontendPluginRegistry::Add<FindBadConstructsAction>
 X("find-bad-constructs", "Finds bad C++ constructs");