Skip to content

Commit

Permalink
[clang-tidy] modernize-use-override new option AllowOverrideAndFinal
Browse files Browse the repository at this point in the history
Summary:
In addition to adding `override` wherever possible, clang-tidy's `modernize-use-override` nicely removes `virtual` when `override` or `final` is specified, and further removes override when final is specified. While this is great default behavior, when code needs to be compiled with gcc at high warning levels that include `gcc -Wsuggest-override` or `gcc -Werror=suggest-override`, clang-tidy's removal of the redundant `override` keyword causes gcc to emit a warning or error. This discrepancy / conflict has been noted by others including a comment on Stack Overflow and by Mozilla's Firefox developers.

This patch adds an AllowOverrideAndFinal option defaulting to 0 - thus preserving current behavior - that when enabled allows both `override` and `final` to co-exist, while still fixing all other issues.

The patch includes a test file verifying all combinations of virtual/override/final, and mentions the new option in the release notes.

Reviewers: alexfh, djasper, JonasToth

Patch by: poelmanc

Subscribers: JonasToth, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D70165
  • Loading branch information
Mitchell Balan committed Nov 15, 2019
1 parent ee0882b commit 50e9956
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,23 @@ void RedundantStringInitCheck::registerMatchers(MatchFinder *Finder) {
// string bar("");
Finder->addMatcher(
namedDecl(
varDecl(hasType(hasUnqualifiedDesugaredType(recordType(
hasDeclaration(cxxRecordDecl(hasName("basic_string")))))),
hasInitializer(expr(ignoringImplicit(anyOf(
EmptyStringCtorExpr,
EmptyStringCtorExprWithTemporaries)))
.bind("expr"))),
unless(parmVarDecl()))
.bind("decl"),
varDecl(
hasType(hasUnqualifiedDesugaredType(recordType(
hasDeclaration(cxxRecordDecl(hasName("basic_string")))))),
hasInitializer(expr(ignoringImplicit(anyOf(
EmptyStringCtorExpr, EmptyStringCtorExprWithTemporaries)))))
.bind("vardecl"),
unless(parmVarDecl())),
this);
}

void RedundantStringInitCheck::check(const MatchFinder::MatchResult &Result) {
const auto *CtorExpr = Result.Nodes.getNodeAs<Expr>("expr");
const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl");
diag(CtorExpr->getExprLoc(), "redundant string initialization")
<< FixItHint::CreateReplacement(CtorExpr->getSourceRange(),
Decl->getName());
const auto *VDecl = Result.Nodes.getNodeAs<VarDecl>("vardecl");
// VarDecl's getSourceRange() spans 'string foo = ""' or 'string bar("")'.
// So start at getLocation() to span just 'foo = ""' or 'bar("")'.
SourceRange ReplaceRange(VDecl->getLocation(), VDecl->getEndLoc());
diag(VDecl->getLocation(), "redundant string initialization")
<< FixItHint::CreateReplacement(ReplaceRange, VDecl->getName());
}

} // namespace readability
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
// FIXME: Fix the checker to work in C++17 mode.
// RUN: %check_clang_tidy %s readability-redundant-string-init %t

namespace std {
template <typename T>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// RUN: %check_clang_tidy -std=c++11,c++14 %s readability-redundant-string-init %t
// FIXME: Fix the checker to work in C++17 mode.
// RUN: %check_clang_tidy %s readability-redundant-string-init %t

namespace std {
template <typename T>
Expand Down Expand Up @@ -131,6 +130,11 @@ void k() {
// CHECK-FIXES: std::string a, b, c;

std::string d = "u", e = "u", f = "u";

std::string g = "u", h = "", i = "uuu", j = "", k;
// CHECK-MESSAGES: [[@LINE-1]]:24: warning: redundant string initialization
// CHECK-MESSAGES: [[@LINE-2]]:43: warning: redundant string initialization
// CHECK-FIXES: std::string g = "u", h, i = "uuu", j, k;
}

// These cases should not generate warnings.
Expand Down

0 comments on commit 50e9956

Please sign in to comment.