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

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

Issue 1141793003: Update from https://crrev.com/329939 (Closed) Base URL: git@github.com:domokit/mojo.git@master
Patch Set: Created 5 years, 7 months 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 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();
« no previous file with comments | « tools/clang/plugins/FindBadConstructsConsumer.h ('k') | tools/clang/plugins/SuppressibleDiagnosticBuilder.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698