diff options
| -rw-r--r-- | tools/clang/plugins/ChromeClassTester.cpp | 261 | ||||
| -rw-r--r-- | tools/clang/plugins/ChromeClassTester.h | 29 | ||||
| -rw-r--r-- | tools/clang/plugins/FindBadConstructs.cpp | 138 | ||||
| -rw-r--r-- | tools/clang/plugins/tests/base_refcounted.cpp | 72 | ||||
| -rw-r--r-- | tools/clang/plugins/tests/base_refcounted.h | 121 | ||||
| -rw-r--r-- | tools/clang/plugins/tests/base_refcounted.txt | 23 | ||||
| -rw-r--r-- | tools/clang/plugins/tests/inline_ctor.h | 8 | ||||
| -rwxr-xr-x | tools/clang/scripts/plugin_flags.sh | 5 |
8 files changed, 502 insertions, 155 deletions
diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp index dd983bd..7a3b89e 100644 --- a/tools/clang/plugins/ChromeClassTester.cpp +++ b/tools/clang/plugins/ChromeClassTester.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -40,72 +40,6 @@ ChromeClassTester::ChromeClassTester(CompilerInstance& instance) BuildBannedLists(); } -void ChromeClassTester::BuildBannedLists() { - banned_namespaces_.push_back("std"); - banned_namespaces_.push_back("__gnu_cxx"); - banned_namespaces_.push_back("WebKit"); - - banned_directories_.push_back("third_party/"); - banned_directories_.push_back("native_client/"); - banned_directories_.push_back("breakpad/"); - banned_directories_.push_back("courgette/"); - banned_directories_.push_back("pdf/"); - banned_directories_.push_back("ppapi/"); - banned_directories_.push_back("usr/"); - banned_directories_.push_back("testing/"); - banned_directories_.push_back("googleurl/"); - banned_directories_.push_back("v8/"); - banned_directories_.push_back("dart/"); - banned_directories_.push_back("sdch/"); - banned_directories_.push_back("icu4c/"); - banned_directories_.push_back("frameworks/"); - - // Don't check autogenerated headers. - // Make puts them below $(builddir_name)/.../gen and geni. - // Ninja puts them below OUTPUT_DIR/.../gen - // Xcode has a fixed output directory for everything. - banned_directories_.push_back("gen/"); - banned_directories_.push_back("geni/"); - banned_directories_.push_back("xcodebuild/"); - - // You are standing in a mazy of twisty dependencies, all resolved by - // putting everything in the header. - banned_directories_.push_back("automation/"); - - // Don't check system headers. - banned_directories_.push_back("/Developer/"); - - // Used in really low level threading code that probably shouldn't be out of - // lined. - ignored_record_names_.insert("ThreadLocalBoolean"); - - // A complicated pickle derived struct that is all packed integers. - ignored_record_names_.insert("Header"); - - // Part of the GPU system that uses multiple included header - // weirdness. Never getting this right. - ignored_record_names_.insert("Validators"); - - // Has a UNIT_TEST only constructor. Isn't *terribly* complex... - ignored_record_names_.insert("AutocompleteController"); - ignored_record_names_.insert("HistoryURLProvider"); - - // Because of chrome frame - ignored_record_names_.insert("ReliabilityTestSuite"); - - // Used over in the net unittests. A large enough bundle of integers with 1 - // non-pod class member. Probably harmless. - ignored_record_names_.insert("MockTransaction"); - - // Used heavily in ui_unittests and once in views_unittests. Fixing this - // isn't worth the overhead of an additional library. - ignored_record_names_.insert("TestAnimationDelegate"); - - // 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_.insert("PluginVersionInfo"); -} - ChromeClassTester::~ChromeClassTester() {} void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) { @@ -146,7 +80,8 @@ void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) { } } -void ChromeClassTester::emitWarning(SourceLocation loc, const char* raw_error) { +void ChromeClassTester::emitWarning(SourceLocation loc, + const char* raw_error) { FullSourceLoc full(loc, instance().getSourceManager()); std::string err; err = "[chromium-style] "; @@ -159,10 +94,6 @@ void ChromeClassTester::emitWarning(SourceLocation loc, const char* raw_error) { DiagnosticBuilder B = diagnostic().Report(full, id); } -bool ChromeClassTester::InTestingNamespace(const Decl* record) { - return GetNamespace(record).find("testing") != std::string::npos; -} - bool ChromeClassTester::InBannedNamespace(const Decl* record) { std::string n = GetNamespace(record); if (n != "") { @@ -177,8 +108,88 @@ std::string ChromeClassTester::GetNamespace(const Decl* record) { return GetNamespaceImpl(record->getDeclContext(), ""); } +bool ChromeClassTester::InImplementationFile(SourceLocation record_location) { + std::string filename; + if (!GetFilename(record_location, &filename)) { + return false; + } + + if (ends_with(filename, ".cc") || ends_with(filename, ".cpp") || + ends_with(filename, ".mm")) { + return true; + } + + return false; +} + +void ChromeClassTester::BuildBannedLists() { + banned_namespaces_.push_back("std"); + banned_namespaces_.push_back("__gnu_cxx"); + banned_namespaces_.push_back("WebKit"); + + banned_directories_.push_back("third_party/"); + banned_directories_.push_back("native_client/"); + banned_directories_.push_back("breakpad/"); + banned_directories_.push_back("courgette/"); + banned_directories_.push_back("pdf/"); + banned_directories_.push_back("ppapi/"); + banned_directories_.push_back("usr/"); + banned_directories_.push_back("testing/"); + banned_directories_.push_back("googleurl/"); + banned_directories_.push_back("v8/"); + banned_directories_.push_back("dart/"); + banned_directories_.push_back("sdch/"); + banned_directories_.push_back("icu4c/"); + banned_directories_.push_back("frameworks/"); + + // Don't check autogenerated headers. + // Make puts them below $(builddir_name)/.../gen and geni. + // Ninja puts them below OUTPUT_DIR/.../gen + // Xcode has a fixed output directory for everything. + banned_directories_.push_back("gen/"); + banned_directories_.push_back("geni/"); + banned_directories_.push_back("xcodebuild/"); + + // You are standing in a mazy of twisty dependencies, all resolved by + // putting everything in the header. + banned_directories_.push_back("automation/"); + + // Don't check system headers. + banned_directories_.push_back("/Developer/"); + + // Used in really low level threading code that probably shouldn't be out of + // lined. + ignored_record_names_.insert("ThreadLocalBoolean"); + + // A complicated pickle derived struct that is all packed integers. + ignored_record_names_.insert("Header"); + + // Part of the GPU system that uses multiple included header + // weirdness. Never getting this right. + ignored_record_names_.insert("Validators"); + + // Has a UNIT_TEST only constructor. Isn't *terribly* complex... + ignored_record_names_.insert("AutocompleteController"); + ignored_record_names_.insert("HistoryURLProvider"); + + // Because of chrome frame + ignored_record_names_.insert("ReliabilityTestSuite"); + + // Used over in the net unittests. A large enough bundle of integers with 1 + // non-pod class member. Probably harmless. + ignored_record_names_.insert("MockTransaction"); + + // Used heavily in ui_unittests and once in views_unittests. Fixing this + // isn't worth the overhead of an additional library. + ignored_record_names_.insert("TestAnimationDelegate"); + + // 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_.insert("PluginVersionInfo"); +} + std::string ChromeClassTester::GetNamespaceImpl(const DeclContext* context, - std::string candidate) { + const std::string& candidate) { switch (context->getDeclKind()) { case Decl::TranslationUnit: { return candidate; @@ -201,60 +212,49 @@ std::string ChromeClassTester::GetNamespaceImpl(const DeclContext* context, } bool ChromeClassTester::InBannedDirectory(SourceLocation loc) { - const SourceManager &SM = instance_.getSourceManager(); - SourceLocation spelling_location = SM.getSpellingLoc(loc); - PresumedLoc ploc = SM.getPresumedLoc(spelling_location); - std::string buffer_name; - if (ploc.isInvalid()) { - // If we're in an invalid location, we're looking at things that aren't - // actually stated in the source; treat this as a banned location instead - // of going through our full lookup process. + std::string filename; + if (!GetFilename(loc, &filename)) { + // If the filename cannot be determined, simply treat this as a banned + // location, instead of going through the full lookup process. return true; - } else { - std::string b = ploc.getFilename(); - - // We need to special case scratch space; which is where clang does its - // macro expansion. We explicitly want to allow people to do otherwise bad - // things through macros that were defined due to third party libraries. - if (b == "<scratch space>") - return true; - - // Don't complain about these things in implementation files. - if (ends_with(b, ".cc") || ends_with(b, ".cpp") || ends_with(b, ".mm")) { - return true; - } + } - // Don't complain about autogenerated protobuf files. - if (ends_with(b, ".pb.h")) { - return true; - } + // We need to special case scratch space; which is where clang does its + // macro expansion. We explicitly want to allow people to do otherwise bad + // things through macros that were defined due to third party libraries. + if (filename == "<scratch space>") + return true; - // We need to munge the paths so that they are relative to the repository - // srcroot. We first resolve the symlinktastic relative path and then - // remove our known srcroot from it if needed. - char resolvedPath[MAXPATHLEN]; - if (realpath(b.c_str(), resolvedPath)) { - b = resolvedPath; - } + // Don't complain about autogenerated protobuf files. + if (ends_with(filename, ".pb.h")) { + return true; + } + + // We need to munge the paths so that they are relative to the repository + // srcroot. We first resolve the symlinktastic relative path and then + // remove our known srcroot from it if needed. + char resolvedPath[MAXPATHLEN]; + if (realpath(filename.c_str(), resolvedPath)) { + filename = resolvedPath; + } - // On linux, chrome is often checked out to /usr/local/google. Due to the - // "usr" rule in banned_directories_, all diagnostics would be suppressed - // in that case. As a workaround, strip that prefix. - b = lstrip(b, "/usr/local/google"); - - for (std::vector<std::string>::const_iterator it = - banned_directories_.begin(); - it != banned_directories_.end(); ++it) { - // If we can find any of the banned path components in this path, then - // this file is rejected. - size_t index = b.find(*it); - if (index != std::string::npos) { - bool matches_full_dir_name = index == 0 || b[index - 1] == '/'; - if ((*it)[0] == '/') - matches_full_dir_name = true; - if (matches_full_dir_name) - return true; - } + // On linux, chrome is often checked out to /usr/local/google. Due to the + // "usr" rule in banned_directories_, all diagnostics would be suppressed + // in that case. As a workaround, strip that prefix. + filename = lstrip(filename, "/usr/local/google"); + + for (std::vector<std::string>::const_iterator it = + banned_directories_.begin(); + it != banned_directories_.end(); ++it) { + // If we can find any of the banned path components in this path, then + // this file is rejected. + size_t index = filename.find(*it); + if (index != std::string::npos) { + bool matches_full_dir_name = index == 0 || filename[index - 1] == '/'; + if ((*it)[0] == '/') + matches_full_dir_name = true; + if (matches_full_dir_name) + return true; } } @@ -264,3 +264,18 @@ bool ChromeClassTester::InBannedDirectory(SourceLocation loc) { bool ChromeClassTester::IsIgnoredType(const std::string& base_name) { return ignored_record_names_.find(base_name) != ignored_record_names_.end(); } + +bool ChromeClassTester::GetFilename(SourceLocation loc, + std::string* filename) { + const SourceManager &SM = instance_.getSourceManager(); + SourceLocation spelling_location = SM.getSpellingLoc(loc); + PresumedLoc ploc = SM.getPresumedLoc(spelling_location); + if (ploc.isInvalid()) { + // If we're in an invalid location, we're looking at things that aren't + // actually stated in the source. + return false; + } + + *filename = ploc.getFilename(); + return true; +} diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h index 5004f56..5994a71 100644 --- a/tools/clang/plugins/ChromeClassTester.h +++ b/tools/clang/plugins/ChromeClassTester.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -21,8 +21,6 @@ class ChromeClassTester : public clang::ASTConsumer { explicit ChromeClassTester(clang::CompilerInstance& instance); virtual ~ChromeClassTester(); - void BuildBannedLists(); - // ASTConsumer: virtual void HandleTagDeclDefinition(clang::TagDecl* tag); @@ -34,29 +32,38 @@ 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(const clang::Decl* record); - // Utility method for subclasses to check if this class is in a banned // namespace. bool InBannedNamespace(const clang::Decl* record); + // Utility method for subclasses to determine the namespace of the + // specified record, if any. Unnamed namespaces will be identified as + // "<anonymous namespace>". + std::string GetNamespace(const clang::Decl* record); + + // Utility method for subclasses to check if this class is within an + // implementation (.cc, .cpp, .mm) file. + bool InImplementationFile(clang::SourceLocation location); + private: + void BuildBannedLists(); + // Filtered versions of tags that are only called with things defined in // chrome header files. - virtual void CheckChromeClass(const clang::SourceLocation& record_location, + virtual void CheckChromeClass(clang::SourceLocation record_location, clang::CXXRecordDecl* record) = 0; // Utility methods used for filtering out non-chrome classes (and ones we // deliberately ignore) in HandleTagDeclDefinition(). - std::string GetNamespace(const clang::Decl* record); std::string GetNamespaceImpl(const clang::DeclContext* context, - std::string candidate); + const std::string& candidate); bool InBannedDirectory(clang::SourceLocation loc); bool IsIgnoredType(const std::string& base_name); + // Attempts to determine the filename for the given SourceLocation. + // Returns false if the filename could not be determined. + bool GetFilename(clang::SourceLocation loc, std::string* filename); + clang::CompilerInstance& instance_; clang::DiagnosticsEngine& diagnostic_; diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp index 4b2f800..193fc84 100644 --- a/tools/clang/plugins/FindBadConstructs.cpp +++ b/tools/clang/plugins/FindBadConstructs.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. @@ -10,14 +10,13 @@ // - Missing "virtual" keywords on methods that should be virtual. // - Non-annotated overriding virtual methods. // - Virtual methods with nonempty implementations in their headers. -// -// Things that are still TODO: -// - Deriving from base::RefCounted and friends should mandate non-public -// destructors. +// - Classes that derive from base::RefCounted / base::RefCountedThreadSafe +// should have protected or private destructors. #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,20 +35,109 @@ bool TypeHasNonTrivialDtor(const Type* type) { return false; } +// Returns the underlying Type for |type| by expanding typedefs and removing +// any namespace qualifiers. +const Type* UnwrapType(const Type* type) { + if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type)) + 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) - : ChromeClassTester(instance) {} + FindBadConstructsConsumer(CompilerInstance& instance, + bool check_refcounted_dtors) + : ChromeClassTester(instance), + check_refcounted_dtors_(check_refcounted_dtors) { + } - virtual void CheckChromeClass(const SourceLocation& record_location, + virtual void CheckChromeClass(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); + + // Check that all virtual methods are marked accordingly with both + // virtual and OVERRIDE. + CheckVirtualMethods(record_location, record); + } + + if (check_refcounted_dtors_) + CheckRefCountedDtors(record_location, record); + } + + private: + bool check_refcounted_dtors_; + + // 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); + + const TemplateSpecializationType* base_type = + dyn_cast<TemplateSpecializationType>( + UnwrapType(base->getType().getTypePtr())); + if (!base_type) { + // Base-most definition is not a template, so this cannot derive from + // base::RefCounted. However, it may still be possible to use with a + // scoped_refptr<> and support ref-counting, so this is not a perfect + // guarantee of safety. + return false; + } + + TemplateName name = base_type->getTemplateName(); + if (TemplateDecl* decl = name.getAsTemplateDecl()) { + std::string base_name = decl->getNameAsString(); + + // Check for both base::RefCounted and base::RefCountedThreadSafe. + if (base_name.compare(0, 10, "RefCounted") == 0 && + self->GetNamespace(decl) == "base") { + return true; + } + } + return false; + } + + // Prints errors if the destructor of a RefCounted class is public. + void CheckRefCountedDtors(SourceLocation record_location, + CXXRecordDecl* record) { + // Skip anonymous structs. + if (record->getIdentifier() == NULL) + return; + + CXXBasePaths paths; + if (!record->lookupInBases( + &FindBadConstructsConsumer::IsRefCountedCallback, this, 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. - void CheckCtorDtorWeight(const SourceLocation& record_location, + void CheckCtorDtorWeight(SourceLocation record_location, CXXRecordDecl* record) { // We don't handle anonymous structs. If this record doesn't have a // name, it's of the form: @@ -161,6 +249,10 @@ class FindBadConstructsConsumer : public ChromeClassTester { } } + bool InTestingNamespace(const Decl* record) { + return GetNamespace(record).find("testing") != std::string::npos; + } + bool IsMethodInBannedNamespace(const CXXMethodDecl* method) { if (InBannedNamespace(method)) return true; @@ -196,7 +288,7 @@ class FindBadConstructsConsumer : public ChromeClassTester { // 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, + void CheckVirtualMethods(SourceLocation record_location, CXXRecordDecl* record) { for (CXXRecordDecl::field_iterator it = record->field_begin(); it != record->field_end(); ++it) { @@ -283,16 +375,32 @@ class FindBadConstructsConsumer : public ChromeClassTester { }; class FindBadConstructsAction : public PluginASTAction { + public: + FindBadConstructsAction() : check_refcounted_dtors_(true) {} + protected: ASTConsumer* CreateASTConsumer(CompilerInstance &CI, llvm::StringRef ref) { - return new FindBadConstructsConsumer(CI); + return new FindBadConstructsConsumer(CI, check_refcounted_dtors_); } bool ParseArgs(const CompilerInstance &CI, const std::vector<std::string>& args) { - // We don't take any additional arguments here. - return true; + bool parsed = true; + + for (size_t i = 0; i < args.size() && parsed; ++i) { + if (args[i] == "skip-refcounted-dtors") { + check_refcounted_dtors_ = false; + } else { + parsed = false; + llvm::errs() << "Unknown argument: " << args[i] << "\n"; + } + } + + return parsed; } + + private: + bool check_refcounted_dtors_; }; } // namespace diff --git a/tools/clang/plugins/tests/base_refcounted.cpp b/tools/clang/plugins/tests/base_refcounted.cpp new file mode 100644 index 0000000..364a3e8 --- /dev/null +++ b/tools/clang/plugins/tests/base_refcounted.cpp @@ -0,0 +1,72 @@ +// 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 "base_refcounted.h" + +#include <cstddef> + +namespace { + +// Unsafe; should error. +class AnonymousDerivedProtectedToPublicInImpl + : public ProtectedRefCountedDtorInHeader { + public: + AnonymousDerivedProtectedToPublicInImpl() {} + ~AnonymousDerivedProtectedToPublicInImpl() {} +}; + +} // namespace + +// Unsafe; should error. +class PublicRefCountedDtorInImpl + : public base::RefCounted<PublicRefCountedDtorInImpl> { + public: + PublicRefCountedDtorInImpl() {} + ~PublicRefCountedDtorInImpl() {} + + private: + friend class base::RefCounted<PublicRefCountedDtorInImpl>; +}; + +class Foo { + public: + class BarInterface { + protected: + virtual ~BarInterface() {} + }; + + typedef base::RefCounted<BarInterface> RefCountedBar; + typedef RefCountedBar AnotherTypedef; +}; + +class Baz { + public: + typedef typename Foo::AnotherTypedef MyLocalTypedef; +}; + +// Unsafe; should error. +class UnsafeTypedefChainInImpl : public Baz::MyLocalTypedef { + public: + UnsafeTypedefChainInImpl() {} + ~UnsafeTypedefChainInImpl() {} +}; + +int main() { + PublicRefCountedDtorInHeader bad; + PublicRefCountedDtorInImpl also_bad; + + ProtectedRefCountedDtorInHeader* protected_ok = NULL; + PrivateRefCountedDtorInHeader* private_ok = NULL; + + DerivedProtectedToPublicInHeader still_bad; + PublicRefCountedThreadSafeDtorInHeader another_bad_variation; + AnonymousDerivedProtectedToPublicInImpl and_this_is_bad_too; + ImplicitDerivedProtectedToPublicInHeader bad_yet_again; + UnsafeTypedefChainInImpl and_again_this_is_bad; + + WebKitPublicDtorInHeader ignored; + WebKitDerivedPublicDtorInHeader still_ignored; + + return 0; +} diff --git a/tools/clang/plugins/tests/base_refcounted.h b/tools/clang/plugins/tests/base_refcounted.h new file mode 100644 index 0000000..1e53215 --- /dev/null +++ b/tools/clang/plugins/tests/base_refcounted.h @@ -0,0 +1,121 @@ +// 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. + +#ifndef BASE_REFCOUNTED_H_ +#define BASE_REFCOUNTED_H_ + +namespace base { + +template <typename T> +class RefCounted { + public: + RefCounted() {} + ~RefCounted() {} +}; + +template <typename T> +class RefCountedThreadSafe { + public: + RefCountedThreadSafe() {} + ~RefCountedThreadSafe() {} +}; + +} // namespace base + +// Ignore classes whose inheritance tree ends in WebKit's RefCounted base +// class. Though prone to error, this pattern is very prevalent in WebKit +// code, so do not issue any warnings. +namespace WebKit { + +template <typename T> +class RefCounted { + public: + RefCounted() {} + ~RefCounted() {} +}; + +} // namespace WebKit + +// Unsafe; should error. +class PublicRefCountedDtorInHeader + : public base::RefCounted<PublicRefCountedDtorInHeader> { + public: + PublicRefCountedDtorInHeader() {} + ~PublicRefCountedDtorInHeader() {} + + private: + friend class base::RefCounted<PublicRefCountedDtorInHeader>; +}; + +// Unsafe; should error. +class PublicRefCountedThreadSafeDtorInHeader + : public base::RefCountedThreadSafe< + PublicRefCountedThreadSafeDtorInHeader> { + public: + PublicRefCountedThreadSafeDtorInHeader() {} + ~PublicRefCountedThreadSafeDtorInHeader() {} + + private: + friend class base::RefCountedThreadSafe< + PublicRefCountedThreadSafeDtorInHeader>; +}; + +// Safe; should not have errors. +class ProtectedRefCountedDtorInHeader + : public base::RefCounted<ProtectedRefCountedDtorInHeader> { + public: + ProtectedRefCountedDtorInHeader() {} + + protected: + ~ProtectedRefCountedDtorInHeader() {} + + private: + friend class base::RefCounted<ProtectedRefCountedDtorInHeader>; +}; + +// Safe; should not have errors. +class PrivateRefCountedDtorInHeader + : public base::RefCounted<PrivateRefCountedDtorInHeader> { + public: + PrivateRefCountedDtorInHeader() {} + + private: + ~PrivateRefCountedDtorInHeader() {} + friend class base::RefCounted<PrivateRefCountedDtorInHeader>; +}; + +// Unsafe; A grandchild class ends up exposing their parent and grandparent's +// destructors. +class DerivedProtectedToPublicInHeader + : public ProtectedRefCountedDtorInHeader { + public: + DerivedProtectedToPublicInHeader() {} + ~DerivedProtectedToPublicInHeader() {} +}; + +// Unsafe; A grandchild ends up implicitly exposing their parent and +// grantparent's destructors. +class ImplicitDerivedProtectedToPublicInHeader + : public ProtectedRefCountedDtorInHeader { + public: + ImplicitDerivedProtectedToPublicInHeader() {} +}; + +// Unsafe-but-ignored; should not have errors. +class WebKitPublicDtorInHeader + : public WebKit::RefCounted<WebKitPublicDtorInHeader> { + public: + WebKitPublicDtorInHeader() {} + ~WebKitPublicDtorInHeader() {} +}; + +// Unsafe-but-ignored; should not have errors. +class WebKitDerivedPublicDtorInHeader + : public WebKitPublicDtorInHeader { + public: + WebKitDerivedPublicDtorInHeader() {} + ~WebKitDerivedPublicDtorInHeader() {} +}; + +#endif // BASE_REFCOUNTED_H_ diff --git a/tools/clang/plugins/tests/base_refcounted.txt b/tools/clang/plugins/tests/base_refcounted.txt new file mode 100644 index 0000000..4626424 --- /dev/null +++ b/tools/clang/plugins/tests/base_refcounted.txt @@ -0,0 +1,23 @@ +In file included from base_refcounted.cpp:5: +./base_refcounted.h:45:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors. + ~PublicRefCountedDtorInHeader() {} + ^ +./base_refcounted.h:57:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors. + ~PublicRefCountedThreadSafeDtorInHeader() {} + ^ +./base_refcounted.h:94:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors. + ~DerivedProtectedToPublicInHeader() {} + ^ +./base_refcounted.h:99:1: warning: [chromium-style] Classes that are ref-counted should have explicit destructors that are protected or private. +class ImplicitDerivedProtectedToPublicInHeader +^ +base_refcounted.cpp:16:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors. + ~AnonymousDerivedProtectedToPublicInImpl() {} + ^ +base_refcounted.cpp:26:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors. + ~PublicRefCountedDtorInImpl() {} + ^ +base_refcounted.cpp:52:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors. + ~UnsafeTypedefChainInImpl() {} + ^ +7 warnings generated. diff --git a/tools/clang/plugins/tests/inline_ctor.h b/tools/clang/plugins/tests/inline_ctor.h index ce2685e..d053b2f 100644 --- a/tools/clang/plugins/tests/inline_ctor.h +++ b/tools/clang/plugins/tests/inline_ctor.h @@ -1,9 +1,9 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// 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. -#ifndef INCLINE_CTOR_H_ -#define INCLINE_CTOR_H_ +#ifndef INLINE_CTOR_H_ +#define INLINE_CTOR_H_ #include <string> #include <vector> @@ -18,4 +18,4 @@ class InlineCtorsArentOKInHeader { std::vector<std::string> two_; }; -#endif // INCLINE_CTOR_H_ +#endif // INLINE_CTOR_H_ diff --git a/tools/clang/scripts/plugin_flags.sh b/tools/clang/scripts/plugin_flags.sh index ec3eed6c..bd9f547 100755 --- a/tools/clang/scripts/plugin_flags.sh +++ b/tools/clang/scripts/plugin_flags.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Copyright (c) 2011 The Chromium Authors. All rights reserved. +# 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. @@ -17,4 +17,5 @@ else fi echo -Xclang -load -Xclang $CLANG_LIB_PATH/libFindBadConstructs.$LIBSUFFIX \ - -Xclang -add-plugin -Xclang find-bad-constructs + -Xclang -add-plugin -Xclang find-bad-constructs \ + -Xclang -plugin-arg-find-bad-constructs -Xclang skip-refcounted-dtors |