Index: tools/clang/plugins/FindBadConstructsConsumer.cpp |
diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp |
index 4dc305e4a4e3d61bd751ce65f44812348e8335bc..83a18b0206f7d1ac6371a5fead21a7ba05f91aa9 100644 |
--- a/tools/clang/plugins/FindBadConstructsConsumer.cpp |
+++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp |
@@ -78,13 +78,14 @@ bool IsGtestTestFixture(const CXXRecordDecl* decl) { |
// have to be true, but it probably will be. |
// TODO(dcheng): Add a warning to force virtual to always appear first ;-) |
FixItHint FixItRemovalForVirtual(const SourceManager& manager, |
+ const LangOptions& lang_opts, |
const CXXMethodDecl* method) { |
SourceRange range(method->getLocStart()); |
// Get the spelling loc just in case it was expanded from a macro. |
SourceRange spelling_range(manager.getSpellingLoc(range.getBegin())); |
// Sanity check that the text looks like virtual. |
StringRef text = clang::Lexer::getSourceText( |
- CharSourceRange::getTokenRange(spelling_range), manager, LangOptions()); |
+ CharSourceRange::getTokenRange(spelling_range), manager, lang_opts); |
if (text.trim() != "virtual") |
return FixItHint(); |
return FixItHint::CreateRemoval(range); |
@@ -290,12 +291,30 @@ void FindBadConstructsConsumer::CheckCtorDtorWeight( |
if (it->hasInlineBody()) { |
if (it->isCopyConstructor() && |
!record->hasUserDeclaredCopyConstructor()) { |
- emitWarning(record_location, |
- "Complex class/struct needs an explicit out-of-line " |
- "copy constructor."); |
+ // In general, implicit constructors are generated on demand. But |
+ // in the Windows component build, dllexport causes instantiation of |
+ // the copy constructor which means that this fires on many more |
+ // classes. For now, suppress this on dllexported classes. |
+ // (This does mean that windows component builds will not emit this |
+ // warning in some cases where it is emitted in other configs, but |
+ // that's the better tradeoff at this point). |
+ // TODO(dcheng): With the RecursiveASTVisitor, these warnings might |
+ // be emitted on other platforms too, reevaluate if we want to keep |
+ // surpressing this then http://crbug.com/467288 |
+ if (!record->hasAttr<DLLExportAttr>()) |
+ emitWarning(record_location, |
+ "Complex class/struct needs an explicit out-of-line " |
+ "copy constructor."); |
} else { |
- emitWarning(it->getInnerLocStart(), |
- "Complex constructor has an inlined body."); |
+ // See the comment in the previous branch about copy constructors. |
+ // This does the same for implicit move constructors. |
+ bool is_likely_compiler_generated_dllexport_move_ctor = |
+ it->isMoveConstructor() && |
+ !record->hasUserDeclaredMoveConstructor() && |
+ record->hasAttr<DLLExportAttr>(); |
+ if (!is_likely_compiler_generated_dllexport_move_ctor) |
+ emitWarning(it->getInnerLocStart(), |
+ "Complex constructor has an inlined body."); |
} |
} else if (it->isInlined() && !it->isInlineSpecified() && |
!it->isDeleted() && (!it->isCopyOrMoveConstructor() || |
@@ -354,6 +373,15 @@ bool FindBadConstructsConsumer::IsMethodInBannedOrTestingNamespace( |
return false; |
} |
+SuppressibleDiagnosticBuilder |
+FindBadConstructsConsumer::ReportIfSpellingLocNotIgnored( |
+ SourceLocation loc, |
+ unsigned diagnostic_id) { |
+ return SuppressibleDiagnosticBuilder( |
+ &diagnostic(), loc, diagnostic_id, |
+ InBannedDirectory(instance().getSourceManager().getSpellingLoc(loc))); |
+} |
+ |
// Checks that virtual methods are correctly annotated, and have no body in a |
// header file. |
void FindBadConstructsConsumer::CheckVirtualMethods( |
@@ -411,6 +439,7 @@ void FindBadConstructsConsumer::CheckVirtualSpecifiers( |
return; |
SourceManager& manager = instance().getSourceManager(); |
+ const LangOptions& lang_opts = instance().getLangOpts(); |
// Complain if a method is annotated virtual && (override || final). |
if (has_virtual && (override_attr || final_attr)) { |
@@ -418,13 +447,11 @@ void FindBadConstructsConsumer::CheckVirtualSpecifiers( |
// Note this is just an educated guess: the assumption here is that any |
// macro for declaring methods will probably be at the start of the method's |
// source range. |
- if (!InBannedDirectory(manager.getSpellingLoc(method->getLocStart()))) { |
- diagnostic().Report(method->getLocStart(), |
- diag_redundant_virtual_specifier_) |
- << "'virtual'" |
- << (override_attr ? static_cast<Attr*>(override_attr) : final_attr) |
- << FixItRemovalForVirtual(manager, method); |
- } |
+ ReportIfSpellingLocNotIgnored(method->getLocStart(), |
+ diag_redundant_virtual_specifier_) |
+ << "'virtual'" |
+ << (override_attr ? static_cast<Attr*>(override_attr) : final_attr) |
+ << FixItRemovalForVirtual(manager, lang_opts, method); |
} |
// Complain if a method is an override and is not annotated with override or |
@@ -435,11 +462,8 @@ void FindBadConstructsConsumer::CheckVirtualSpecifiers( |
if (method->hasInlineBody()) { |
loc = method->getBody()->getSourceRange().getBegin(); |
} else { |
- // TODO(dcheng): We should probably use ASTContext's LangOptions here. |
- LangOptions lang_options; |
- loc = Lexer::getLocForEndOfToken( |
- manager.getSpellingLoc(range.getEnd()), 0, |
- manager, lang_options); |
+ loc = Lexer::getLocForEndOfToken(manager.getSpellingLoc(range.getEnd()), |
+ 0, manager, lang_opts); |
// The original code used the ending source loc of TypeSourceInfo's |
// TypeLoc. Unfortunately, this breaks down in the presence of attributes. |
// Attributes often appear at the end of a TypeLoc, e.g. |
@@ -455,11 +479,11 @@ void FindBadConstructsConsumer::CheckVirtualSpecifiers( |
for (SourceLocation l = loc.getLocWithOffset(-1); |
l != manager.getLocForStartOfFile(manager.getFileID(loc)); |
l = l.getLocWithOffset(-1)) { |
- l = Lexer::GetBeginningOfToken(l, manager, lang_options); |
+ l = Lexer::GetBeginningOfToken(l, manager, lang_opts); |
Token token; |
// getRawToken() returns *true* on failure. In that case, just give up |
// and don't bother generating a possibly incorrect fix-it. |
- if (Lexer::getRawToken(l, token, manager, lang_options, true)) { |
+ if (Lexer::getRawToken(l, token, manager, lang_opts, true)) { |
loc = SourceLocation(); |
break; |
} |
@@ -471,25 +495,28 @@ void FindBadConstructsConsumer::CheckVirtualSpecifiers( |
} |
} |
} |
+ // Again, only emit the warning if it doesn't originate from a macro in |
+ // a system header. |
if (loc.isValid()) { |
- diagnostic().Report(loc, diag_method_requires_override_) |
+ ReportIfSpellingLocNotIgnored(loc, diag_method_requires_override_) |
<< FixItHint::CreateInsertion(loc, " override"); |
} else { |
- diagnostic().Report(range.getBegin(), diag_method_requires_override_); |
+ ReportIfSpellingLocNotIgnored(range.getBegin(), |
+ diag_method_requires_override_); |
} |
} |
if (final_attr && override_attr) { |
- diagnostic().Report(override_attr->getLocation(), |
- diag_redundant_virtual_specifier_) |
+ ReportIfSpellingLocNotIgnored(override_attr->getLocation(), |
+ diag_redundant_virtual_specifier_) |
<< override_attr << final_attr |
<< FixItHint::CreateRemoval(override_attr->getRange()); |
} |
if (final_attr && !is_override) { |
- diagnostic().Report(method->getLocStart(), |
- diag_base_method_virtual_and_final_) |
- << FixItRemovalForVirtual(manager, method) |
+ ReportIfSpellingLocNotIgnored(method->getLocStart(), |
+ diag_base_method_virtual_and_final_) |
+ << FixItRemovalForVirtual(manager, lang_opts, method) |
<< FixItHint::CreateRemoval(final_attr->getRange()); |
} |
} |
@@ -501,9 +528,27 @@ void FindBadConstructsConsumer::CheckVirtualBodies( |
if (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."); |
+ 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."); |
} |
} |
} |
@@ -529,7 +574,7 @@ void FindBadConstructsConsumer::CountType(const Type* type, |
bool whitelisted_template = false; |
// HACK: I'm at a loss about how to get the syntax checker to get |
- // whether a template is exterened or not. For the first pass here, |
+ // whether a template is externed or not. For the first pass here, |
// just do retarded string comparisons. |
if (TemplateDecl* decl = name.getAsTemplateDecl()) { |
std::string base_name = decl->getNameAsString(); |