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, |