Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

Unified Diff: tools/clang/plugins/FindBadConstructsConsumer.cpp

Issue 1493813003: Convert the no-inline-virtuals rule into a constructors rule. Base URL: https://chromium.googlesource.com/chromium/src.git@lkcr
Patch Set: Rebase onto https://codereview.chromium.org/1504033010 Created 5 years ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: tools/clang/plugins/FindBadConstructsConsumer.cpp
diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp
index c79a764eb953be319cb5e83fe766cee2bbaa8a3c..de31588b6815427b6c8f639203278ee2ade864e5 100644
--- a/tools/clang/plugins/FindBadConstructsConsumer.cpp
+++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp
@@ -7,6 +7,7 @@
#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;
@@ -98,6 +99,44 @@ bool IsPodOrTemplateType(const CXXRecordDecl& record) {
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 lea
+ 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,
@@ -165,14 +204,13 @@ void FindBadConstructsConsumer::CheckChromeClass(SourceLocation record_location,
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);
@@ -274,6 +312,43 @@ void FindBadConstructsConsumer::CheckCtorDtorWeight(
// 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
+ 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,
@@ -382,12 +457,10 @@ FindBadConstructsConsumer::ReportIfSpellingLocNotIgnored(
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),
@@ -418,8 +491,6 @@ void FindBadConstructsConsumer::CheckVirtualMethods(
continue;
} else {
CheckVirtualSpecifiers(*it);
- if (warn_on_inline_bodies)
- CheckVirtualBodies(*it);
}
}
}
@@ -521,39 +592,6 @@ void FindBadConstructsConsumer::CheckVirtualSpecifiers(
}
}
-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,

Powered by Google App Engine
This is Rietveld 408576698