From 7131dba5f6713c9b7958dee8244858704cb5c57a Mon Sep 17 00:00:00 2001
From: "rsleevi@chromium.org"
<rsleevi@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Tue, 17 Apr 2012 01:31:24 +0000
Subject: Check implementation files for virtual and OVERRIDE.
Currently behind a flag "-Xclang -plugin-arg-find-bad-constructs -Xclang skip-virtuals-in-implementations"
BUG=115047
TEST=none
Review URL: https://chromiumcodereview.appspot.com/10076002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@132507 0039d316-1c4b-4281-b951-d872f2087c98
---
tools/clang/plugins/FindBadConstructs.cpp | 41 +++++++++++++++++-------
tools/clang/plugins/tests/overridden_methods.cpp | 26 ++++++++++++++-
tools/clang/plugins/tests/overridden_methods.txt | 11 ++++++-
tools/clang/plugins/tests/virtual_methods.cpp | 6 ++--
tools/clang/scripts/plugin_flags.sh | 4 ++-
5 files changed, 70 insertions(+), 18 deletions(-)
(limited to 'tools/clang')
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index 193fc84..fee2aad 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -49,9 +49,11 @@ const Type* UnwrapType(const Type* type) {
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,
@@ -62,10 +64,14 @@ class FindBadConstructsConsumer : public ChromeClassTester {
// Only check for "heavy" constructors/destructors in header files;
// within implementation files, there is no performance cost.
CheckCtorDtorWeight(record_location, record);
+ }
+
+ if (!implementation_file || check_virtuals_in_implementations_) {
+ bool warn_on_inline_bodies = !implementation_file;
// Check that all virtual methods are marked accordingly with both
// virtual and OVERRIDE.
- CheckVirtualMethods(record_location, record);
+ CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
}
if (check_refcounted_dtors_)
@@ -74,6 +80,7 @@ class FindBadConstructsConsumer : public ChromeClassTester {
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
@@ -225,7 +232,8 @@ class FindBadConstructsConsumer : public ChromeClassTester {
}
}
- void CheckVirtualMethod(const CXXMethodDecl* method) {
+ void CheckVirtualMethod(const CXXMethodDecl* method,
+ bool warn_on_inline_bodies) {
if (!method->isVirtual())
return;
@@ -236,8 +244,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(
@@ -281,15 +291,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(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 =
@@ -310,7 +320,7 @@ class FindBadConstructsConsumer : public ChromeClassTester {
!record->hasUserDeclaredDestructor()) {
// Ignore non-user-declared destructors.
} else {
- CheckVirtualMethod(*it);
+ CheckVirtualMethod(*it, warn_on_inline_bodies);
CheckOverriddenMethod(*it);
}
}
@@ -376,11 +386,15 @@ class FindBadConstructsConsumer : public ChromeClassTester {
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,
@@ -390,6 +404,8 @@ class FindBadConstructsAction : public PluginASTAction {
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";
@@ -401,6 +417,7 @@ class FindBadConstructsAction : public PluginASTAction {
private:
bool check_refcounted_dtors_;
+ bool check_virtuals_in_implementations_;
};
} // namespace
diff --git a/tools/clang/plugins/tests/overridden_methods.cpp b/tools/clang/plugins/tests/overridden_methods.cpp
index f81d3a2..f572a41 100644
--- a/tools/clang/plugins/tests/overridden_methods.cpp
+++ b/tools/clang/plugins/tests/overridden_methods.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.
@@ -9,6 +9,30 @@ void DerivedClass::SomeMethod() {}
void DerivedClass::SomeOtherMethod() {}
void DerivedClass::WebKitModifiedSomething() {}
+class ImplementationInterimClass : public BaseClass {
+ public:
+ // Should not warn about pure virtual methods.
+ virtual void SomeMethod() = 0;
+};
+
+class ImplementationDerivedClass : public ImplementationInterimClass,
+ public webkit_glue::WebKitObserverImpl {
+ public:
+ // Should not warn about destructors.
+ virtual ~ImplementationDerivedClass() {}
+ // Should warn.
+ virtual void SomeMethod();
+ // Should not warn if marked as override.
+ virtual void SomeOtherMethod() override;
+ // Should not warn for inline implementations in implementation files.
+ virtual void SomeInlineMethod() {}
+ // Should not warn if overriding a method whose origin is WebKit.
+ virtual void WebKitModifiedSomething();
+ // Should warn if overridden method isn't pure.
+ virtual void SomeNonPureBaseMethod() {}
+};
+
int main() {
DerivedClass something;
+ ImplementationDerivedClass something_else;
}
diff --git a/tools/clang/plugins/tests/overridden_methods.txt b/tools/clang/plugins/tests/overridden_methods.txt
index a405d31..7553ade 100644
--- a/tools/clang/plugins/tests/overridden_methods.txt
+++ b/tools/clang/plugins/tests/overridden_methods.txt
@@ -8,4 +8,13 @@ In file included from overridden_methods.cpp:5:
./overridden_methods.h:51:11: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
virtual void SomeNonPureBaseMethod() {}
^
-3 warnings generated.
+overridden_methods.cpp:24:11: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+ virtual void SomeMethod();
+ ^
+overridden_methods.cpp:28:11: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+ virtual void SomeInlineMethod() {}
+ ^
+overridden_methods.cpp:32:11: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+ virtual void SomeNonPureBaseMethod() {}
+ ^
+6 warnings generated.
diff --git a/tools/clang/plugins/tests/virtual_methods.cpp b/tools/clang/plugins/tests/virtual_methods.cpp
index 05515b7..a07cbe48 100644
--- a/tools/clang/plugins/tests/virtual_methods.cpp
+++ b/tools/clang/plugins/tests/virtual_methods.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.
@@ -16,13 +16,13 @@ class VirtualMethodsInImplementation {
// Stubs to fill in the abstract method
class ConcreteVirtualMethodsInHeaders : public VirtualMethodsInHeaders {
public:
- virtual void MethodIsAbstract() {}
+ virtual void MethodIsAbstract() override {}
};
class ConcreteVirtualMethodsInImplementation
: public VirtualMethodsInImplementation {
public:
- virtual void MethodIsAbstract() {}
+ virtual void MethodIsAbstract() override {}
};
// Fill in the implementations
diff --git a/tools/clang/scripts/plugin_flags.sh b/tools/clang/scripts/plugin_flags.sh
index bd9f547..da0ec65 100755
--- a/tools/clang/scripts/plugin_flags.sh
+++ b/tools/clang/scripts/plugin_flags.sh
@@ -18,4 +18,6 @@ fi
echo -Xclang -load -Xclang $CLANG_LIB_PATH/libFindBadConstructs.$LIBSUFFIX \
-Xclang -add-plugin -Xclang find-bad-constructs \
- -Xclang -plugin-arg-find-bad-constructs -Xclang skip-refcounted-dtors
+ -Xclang -plugin-arg-find-bad-constructs -Xclang skip-refcounted-dtors \
+ -Xclang -plugin-arg-find-bad-constructs \
+ -Xclang skip-virtuals-in-implementations
--
cgit v1.1