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

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

Issue 597863002: Update plugin to handle new style rules for virtual annotations. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: s/annotation/specifier/g Created 6 years, 3 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 a3c6e7f4f14b9316dd432cd5f6ff9bf03e1f7320..41c100fe89c13541f78d7b0e7a4d4c3fa12d1ae5 100644
--- a/tools/clang/plugins/FindBadConstructsConsumer.cpp
+++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp
@@ -5,6 +5,7 @@
#include "FindBadConstructsConsumer.h"
#include "clang/Frontend/CompilerInstance.h"
+#include "clang/AST/Attr.h"
#include "clang/Lex/Lexer.h"
#include "llvm/Support/raw_ostream.h"
@@ -15,9 +16,15 @@ namespace chrome_checker {
namespace {
const char kMethodRequiresOverride[] =
- "[chromium-style] Overriding method must be marked with OVERRIDE.";
-const char kMethodRequiresVirtual[] =
- "[chromium-style] Overriding method must have \"virtual\" keyword.";
+ "[chromium-style] Overriding method must be marked with override or final.";
hans 2014/09/26 01:29:29 pretty nitty, and we probably haven't done this so
dcheng 2014/09/26 07:07:13 Done.
+const char kRedundantVirtualSpecifier[] =
+ "[chromium-style] %0 is redundant; %1 implies %0.";
hans 2014/09/26 01:29:29 as per above, I'd put single quotes around the %0'
dcheng 2014/09/26 07:07:14 Done, though I opted to just fix the instance wher
+// http://llvm.org/bugs/show_bug.cgi?id=21051 has been filed to make this a
+// Clang warning.
+const char kBaseMethodVirtualAndFinal[] =
+ "[chromium-style] a virtual method on a base class should not also be "
+ "final; "
+ "method should be devirtualized";
hans 2014/09/26 01:29:29 s/on a base class/in a base class/ I'm not sure i
dcheng 2014/09/26 07:07:13 Updated with your text and weakened the suggestion
const char kNoExplicitDtor[] =
"[chromium-style] Classes that are ref-counted should have explicit "
"destructors that are declared protected or private.";
@@ -59,22 +66,41 @@ const Type* UnwrapType(const Type* type) {
return type;
}
+// Unfortunately, there doesn't seem to be a good way to determine the location
+// of the 'virtual' keyword. It's available in Declarator, but that isn't
+// accessible from the AST. So instead, make an educated guess that the first
+// token is probably the virtual keyword. Strictly speaking, this doesn't have
+// to be true, but it probably will be.
+// TODO(dcheng): Add a warning to force virtual to always appear first ;-)
+SourceRange GuessSourceRangeForVirtual(const SourceManager& manager,
+ const CXXMethodDecl* method) {
hans 2014/09/26 01:29:29 It might be overkill, but you could reach into the
dcheng 2014/09/26 07:07:13 Heh. Done.
+ return SourceRange(method->getLocStart(),
+ Lexer::getLocForEndOfToken(
+ method->getLocStart(), 0, manager, LangOptions()));
+}
+
} // namespace
FindBadConstructsConsumer::FindBadConstructsConsumer(CompilerInstance& instance,
const Options& options)
: ChromeClassTester(instance), options_(options) {
- // Register warning/error messages.
+ // Messages for virtual method specifiers.
diag_method_requires_override_ =
diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresOverride);
- diag_method_requires_virtual_ =
- diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresVirtual);
+ diag_redundant_virtual_specifier_ = diagnostic().getCustomDiagID(
+ getErrorLevel(), kRedundantVirtualSpecifier);
+ diag_base_method_virtual_and_final_ =
+ diagnostic().getCustomDiagID(getErrorLevel(), kBaseMethodVirtualAndFinal);
+
+ // Messages for destructors.
diag_no_explicit_dtor_ =
diagnostic().getCustomDiagID(getErrorLevel(), kNoExplicitDtor);
diag_public_dtor_ =
diagnostic().getCustomDiagID(getErrorLevel(), kPublicDtor);
diag_protected_non_virtual_dtor_ =
diagnostic().getCustomDiagID(getErrorLevel(), kProtectedNonVirtualDtor);
+
+ // Miscellaneous messages.
diag_weak_ptr_factory_order_ =
diagnostic().getCustomDiagID(getErrorLevel(), kWeakPtrFactoryOrder);
diag_bad_enum_last_value_ =
@@ -103,8 +129,7 @@ void FindBadConstructsConsumer::CheckChromeClass(SourceLocation record_location,
bool warn_on_inline_bodies = !implementation_file;
- // Check that all virtual methods are marked accordingly with both
- // virtual and OVERRIDE.
+ // Check that all virtual methods are annotated with override or final.
CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
CheckRefCountedDtors(record_location, record);
@@ -250,35 +275,6 @@ void FindBadConstructsConsumer::CheckCtorDtorWeight(
}
}
-void FindBadConstructsConsumer::CheckVirtualMethod(const CXXMethodDecl* method,
- bool warn_on_inline_bodies) {
- if (!method->isVirtual())
- return;
-
- if (!method->isVirtualAsWritten()) {
- SourceLocation loc = method->getTypeSpecStartLoc();
- if (isa<CXXDestructorDecl>(method))
- loc = method->getInnerLocStart();
- SourceManager& manager = instance().getSourceManager();
- FullSourceLoc full_loc(loc, manager);
- SourceLocation spelling_loc = manager.getSpellingLoc(loc);
- diagnostic().Report(full_loc, diag_method_requires_virtual_)
- << FixItHint::CreateInsertion(spelling_loc, "virtual ");
- }
-
- // 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(cs->getLBracLoc(),
- "virtual methods with non-empty bodies shouldn't be "
- "declared inline.");
- }
- }
- }
-}
-
bool FindBadConstructsConsumer::InTestingNamespace(const Decl* record) {
return GetNamespace(record).find("testing") != std::string::npos;
}
@@ -300,47 +296,16 @@ bool FindBadConstructsConsumer::IsMethodInBannedOrTestingNamespace(
return false;
}
-void FindBadConstructsConsumer::CheckOverriddenMethod(
- const CXXMethodDecl* method) {
- if (!method->size_overridden_methods() || method->getAttr<OverrideAttr>())
- return;
-
- if (isa<CXXDestructorDecl>(method) || method->isPure())
- return;
-
- if (IsMethodInBannedOrTestingNamespace(method))
- return;
-
- SourceManager& manager = instance().getSourceManager();
- SourceRange type_info_range =
- method->getTypeSourceInfo()->getTypeLoc().getSourceRange();
- FullSourceLoc loc(type_info_range.getBegin(), manager);
-
- // Build the FixIt insertion point after the end of the method definition,
- // including any const-qualifiers and attributes, and before the opening
- // of the l-curly-brace (if inline) or the semi-color (if a declaration).
- SourceLocation spelling_end =
- manager.getSpellingLoc(type_info_range.getEnd());
- if (spelling_end.isValid()) {
- SourceLocation token_end =
- Lexer::getLocForEndOfToken(spelling_end, 0, manager, LangOptions());
- diagnostic().Report(token_end, diag_method_requires_override_)
- << FixItHint::CreateInsertion(token_end, " OVERRIDE");
- } else {
- diagnostic().Report(loc, diag_method_requires_override_);
- }
-}
-
-// 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
+// Checks that virtual methods are correctly annotated, and have no body in a
+// header file.
void FindBadConstructsConsumer::CheckVirtualMethods(
SourceLocation record_location,
CXXRecordDecl* record,
bool warn_on_inline_bodies) {
+ // 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
for (CXXRecordDecl::field_iterator it = record->field_begin();
it != record->field_end();
++it) {
@@ -363,9 +328,97 @@ void FindBadConstructsConsumer::CheckVirtualMethods(
} else if (isa<CXXDestructorDecl>(*it) &&
!record->hasUserDeclaredDestructor()) {
// Ignore non-user-declared destructors.
+ } else if (!it->isVirtual()) {
+ continue;
+ } else {
+ CheckVirtualSpecifiers(*it);
+ if (warn_on_inline_bodies)
+ CheckVirtualBodies(*it);
+ }
+ }
+}
+
+// Makes sure that virtual methods use the most appropriate specifier. If a
+// virtual method overrides a method from a base class, the method must use only
+// the override specifier. Furthermore, if the method is not meant to be further
+// subclasses, only the final specifier may be used.
hans 2014/09/26 01:29:29 "not meant to be further subclasses" sounds weird.
dcheng 2014/09/26 07:07:13 Oops, that's a typo. Fixed.
+void FindBadConstructsConsumer::CheckVirtualSpecifiers(
+ const CXXMethodDecl* method) {
+ bool is_override = method->size_overridden_methods() > 0;
+ bool has_virtual = method->isVirtualAsWritten();
+ OverrideAttr* override_attr = method->getAttr<OverrideAttr>();
+ FinalAttr* final_attr = method->getAttr<FinalAttr>();
+
+ if (method->isPure())
+ return;
+
+ if (IsMethodInBannedOrTestingNamespace(method))
+ return;
+
+ if (isa<CXXDestructorDecl>(method) && !options_.strict_virtual_specifiers)
+ return;
+
+ SourceManager& manager = instance().getSourceManager();
+
+ // Complain if a method is annotated virtual && (override || final).
+ if (has_virtual && (override_attr || final_attr) &&
+ options_.strict_virtual_specifiers) {
+ diagnostic().Report(method->getLocStart(),
+ diag_redundant_virtual_specifier_)
+ << "virtual"
+ << (override_attr ? static_cast<Attr*>(override_attr) : final_attr)
+ << FixItHint::CreateRemoval(
+ GuessSourceRangeForVirtual(manager, method));
+ }
+
+ // Complain if a method is an override and is not annotated with override or
+ // final.
+ if (is_override && !override_attr && !final_attr) {
+ SourceRange type_info_range =
+ method->getTypeSourceInfo()->getTypeLoc().getSourceRange();
+ FullSourceLoc loc(type_info_range.getBegin(), manager);
+
+ // Build the FixIt insertion point after the end of the method definition,
+ // including any const-qualifiers and attributes, and before the opening
+ // of the l-curly-brace (if inline) or the semi-color (if a declaration).
+ SourceLocation spelling_end =
+ manager.getSpellingLoc(type_info_range.getEnd());
+ if (spelling_end.isValid()) {
+ SourceLocation token_end =
+ Lexer::getLocForEndOfToken(spelling_end, 0, manager, LangOptions());
+ diagnostic().Report(token_end, diag_method_requires_override_)
+ << FixItHint::CreateInsertion(token_end, " override");
} else {
- CheckVirtualMethod(*it, warn_on_inline_bodies);
- CheckOverriddenMethod(*it);
+ diagnostic().Report(loc, diag_method_requires_override_);
+ }
+ }
+
+ if (final_attr && override_attr && options_.strict_virtual_specifiers) {
+ diagnostic().Report(override_attr->getLocation(),
+ diag_redundant_virtual_specifier_)
+ << override_attr << final_attr
+ << FixItHint::CreateRemoval(override_attr->getRange());
+ }
+
+ if (final_attr && !is_override && options_.strict_virtual_specifiers) {
+ diagnostic().Report(method->getLocStart(),
+ diag_base_method_virtual_and_final_)
+ << FixItHint::CreateRemoval(GuessSourceRangeForVirtual(manager, method))
hans 2014/09/26 01:29:29 Removing the virtual specifier should probably be
dcheng 2014/09/26 07:07:13 It's not needed, because the following does not co
+ << FixItHint::CreateRemoval(final_attr->getRange());
+ }
+}
+
+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()) {
+ emitWarning(cs->getLBracLoc(),
+ "virtual methods with non-empty bodies shouldn't be "
+ "declared inline.");
+ }
}
}
}

Powered by Google App Engine
This is Rietveld 408576698