njames93 updated this revision to Diff 235384.
njames93 edited the summary of this revision.
njames93 added a comment.
Think all the test cases are working, but they look like they could do with a
clean up from someone who may know the clang-tidy-checks system better than
myself
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71846/new/
https://reviews.llvm.org/D71846
Files:
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
clang/docs/LibASTMatchersReference.html
clang/include/clang/ASTMatchers/ASTMatchers.h
clang/lib/ASTMatchers/Dynamic/Registry.cpp
clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===================================================================
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -1071,6 +1071,35 @@
LanguageMode::Cxx17OrLater));
}
+TEST(hasInitStatement, MatchesSelectionInitializers) {
+ EXPECT_TRUE(matches("void baz() { if (int i = 1; i > 0) {} }",
+ ifStmt(hasInitStatement(anything())),
+ LanguageMode::Cxx17OrLater));
+ EXPECT_TRUE(notMatches("void baz() { if (int i = 1) {} }",
+ ifStmt(hasInitStatement(anything()))));
+ EXPECT_TRUE(notMatches("void baz() { if (1 > 0) {} }",
+ ifStmt(hasInitStatement(anything()))));
+ EXPECT_TRUE(matches(
+ "void baz(int i) { switch (int j = i; j) { default: break; } }",
+ switchStmt(hasInitStatement(anything())), LanguageMode::Cxx17OrLater));
+ EXPECT_TRUE(notMatches("void baz(int i) { switch (i) { default: break; } }",
+ switchStmt(hasInitStatement(anything()))));
+}
+
+TEST(hasInitStatement, MatchesRangeForInitializers) {
+ EXPECT_TRUE(matches("void baz() {"
+ "int items[] = {};"
+ "for (auto &arr = items; auto &item : arr) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything())),
+ LanguageMode::Cxx2aOrLater));
+ EXPECT_TRUE(notMatches("void baz() {"
+ "int items[] = {};"
+ "for (auto &item : items) {}"
+ "}",
+ cxxForRangeStmt(hasInitStatement(anything()))));
+}
+
TEST(TemplateArgumentCountIs, Matches) {
EXPECT_TRUE(
matches("template<typename T> struct C {}; C<int> c;",
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===================================================================
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -279,6 +279,7 @@
REGISTER_MATCHER(hasIndex);
REGISTER_MATCHER(hasInit);
REGISTER_MATCHER(hasInitializer);
+ REGISTER_MATCHER(hasInitStatement);
REGISTER_MATCHER(hasKeywordSelector);
REGISTER_MATCHER(hasLHS);
REGISTER_MATCHER(hasLocalQualifiers);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===================================================================
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4297,6 +4297,35 @@
return Node.isConstexpr();
}
+/// Matches selection statements with initializer.
+///
+/// Given:
+/// \code
+/// void foo() {
+/// if (int i = foobar(); i > 0) {}
+/// switch (int i = foobar(); i) {}
+/// for (auto& a = get_range(); auto& x : a) {}
+/// }
+/// void bar() {
+/// if (foobar() > 0) {}
+/// switch (foobar()) {}
+/// for (auto& x : get_range()) {}
+/// }
+/// \endcode
+/// ifStmt(hasInitStatement(anything()))
+/// matches the if statement in foo but not in bar.
+/// switchStmt(hasInitStatement(anything()))
+/// matches the switch statement in foo but not in bar.
+/// cxxForRangeStmt(hasInitStatement(anything()))
+/// matches the range for statement in foo but not in bar.
+AST_POLYMORPHIC_MATCHER_P(hasInitStatement,
+ AST_POLYMORPHIC_SUPPORTED_TYPES(IfStmt, SwitchStmt,
+ CXXForRangeStmt),
+ internal::Matcher<Stmt>, InnerMatcher) {
+ const Stmt *Init = Node.getInit();
+ return Init != nullptr && InnerMatcher.matches(*Init, Finder, Builder);
+}
+
/// Matches the condition expression of an if statement, for loop,
/// switch statement or conditional operator.
///
Index: clang/docs/LibASTMatchersReference.html
===================================================================
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -3009,6 +3009,23 @@
</pre></td></tr>
+<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1Expr.html";>Expr</a>></td><td class="name" onclick="toggle('nullPointerConstant0')"><a name="nullPointerConstant0Anchor">nullPointerConstant</a></td><td></td></tr>
+<tr><td colspan="4" class="doc" id="nullPointerConstant0"><pre>Matches expressions that resolve to a null pointer constant, such as
+GNU's __null, C++11's nullptr, or C's NULL macro.
+
+Given:
+ void *v1 = NULL;
+ void *v2 = nullptr;
+ void *v3 = __null; // GNU extension
+ char *cp = (char *)0;
+ int *ip = 0;
+ int i = 0;
+expr(nullPointerConstant())
+ matches the initializer for v1, v2, v3, cp, and ip. Does not match the
+ initializer for i.
+</pre></td></tr>
+
+
<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1FieldDecl.html";>FieldDecl</a>></td><td class="name" onclick="toggle('hasBitWidth0')"><a name="hasBitWidth0Anchor">hasBitWidth</a></td><td>unsigned Width</td></tr>
<tr><td colspan="4" class="doc" id="hasBitWidth0"><pre>Matches non-static data members that are bit-fields of the specified
bit width.
@@ -3761,6 +3778,18 @@
Example matches y (matcher = parmVarDecl(hasDefaultArgument()))
void x(int val) {}
void y(int val = 0) {}
+
+Deprecated. Use hasInitializer() instead to be able to
+match on the contents of the default argument. For example:
+
+void x(int val = 7) {}
+void y(int val = 42) {}
+parmVarDecl(hasInitializer(integerLiteral(equals(42))))
+ matches the parameter of y
+
+A matcher such as
+ parmVarDecl(hasInitializer(anything()))
+is equivalent to parmVarDecl(hasDefaultArgument()).
</pre></td></tr>
@@ -4479,23 +4508,6 @@
</pre></td></tr>
-<tr><td>Matcher<internal::Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1Expr.html";>Expr</a>>></td><td class="name" onclick="toggle('nullPointerConstant0')"><a name="nullPointerConstant0Anchor">nullPointerConstant</a></td><td></td></tr>
-<tr><td colspan="4" class="doc" id="nullPointerConstant0"><pre>Matches expressions that resolve to a null pointer constant, such as
-GNU's __null, C++11's nullptr, or C's NULL macro.
-
-Given:
- void *v1 = NULL;
- void *v2 = nullptr;
- void *v3 = __null; // GNU extension
- char *cp = (char *)0;
- int *ip = 0;
- int i = 0;
-expr(nullPointerConstant())
- matches the initializer for v1, v2, v3, cp, and ip. Does not match the
- initializer for i.
-</pre></td></tr>
-
-
<tr><td>Matcher<internal::Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1NamedDecl.html";>NamedDecl</a>>></td><td class="name" onclick="toggle('hasAnyName0')"><a name="hasAnyName0Anchor">hasAnyName</a></td><td>StringRef, ..., StringRef</td></tr>
<tr><td colspan="4" class="doc" id="hasAnyName0"><pre>Matches NamedDecl nodes that have any of the specified names.
@@ -5082,6 +5094,29 @@
</pre></td></tr>
+<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXForRangeStmt.html";>CXXForRangeStmt</a>></td><td class="name" onclick="toggle('hasInitStatement2')"><a name="hasInitStatement2Anchor">hasInitStatement</a></td><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html";>Stmt</a>> InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="hasInitStatement2"><pre>Matches selection statements with initializer.
+
+Given:
+ void foo() {
+ if (int i = foobar(); i > 0) {}
+ switch (int i = foobar(); i) {}
+ for (auto& a = get_range(); auto& x : a) {}
+ }
+ void bar() {
+ if (foobar() > 0) {}
+ switch (foobar()) {}
+ for (auto& x : get_range()) {}
+ }
+ifStmt(hasInitStatement(anything()))
+ matches the if statement in foo but not in bar.
+switchStmt(hasInitStatement(anything()))
+ matches the switch statement in foo but not in bar.
+cxxForRangeStmt(hasInitStatement(anything()))
+ matches the range for statement in foo but not in bar.
+</pre></td></tr>
+
+
<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXForRangeStmt.html";>CXXForRangeStmt</a>></td><td class="name" onclick="toggle('hasLoopVariable0')"><a name="hasLoopVariable0Anchor">hasLoopVariable</a></td><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1VarDecl.html";>VarDecl</a>> InnerMatcher</td></tr>
<tr><td colspan="4" class="doc" id="hasLoopVariable0"><pre>Matches the initialization statement of a for loop.
@@ -6206,6 +6241,29 @@
</pre></td></tr>
+<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1IfStmt.html";>IfStmt</a>></td><td class="name" onclick="toggle('hasInitStatement0')"><a name="hasInitStatement0Anchor">hasInitStatement</a></td><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html";>Stmt</a>> InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="hasInitStatement0"><pre>Matches selection statements with initializer.
+
+Given:
+ void foo() {
+ if (int i = foobar(); i > 0) {}
+ switch (int i = foobar(); i) {}
+ for (auto& a = get_range(); auto& x : a) {}
+ }
+ void bar() {
+ if (foobar() > 0) {}
+ switch (foobar()) {}
+ for (auto& x : get_range()) {}
+ }
+ifStmt(hasInitStatement(anything()))
+ matches the if statement in foo but not in bar.
+switchStmt(hasInitStatement(anything()))
+ matches the switch statement in foo but not in bar.
+cxxForRangeStmt(hasInitStatement(anything()))
+ matches the range for statement in foo but not in bar.
+</pre></td></tr>
+
+
<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1IfStmt.html";>IfStmt</a>></td><td class="name" onclick="toggle('hasThen0')"><a name="hasThen0Anchor">hasThen</a></td><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html";>Stmt</a>> InnerMatcher</td></tr>
<tr><td colspan="4" class="doc" id="hasThen0"><pre>Matches the then-statement of an if statement.
@@ -6943,6 +7001,29 @@
</pre></td></tr>
+<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1SwitchStmt.html";>SwitchStmt</a>></td><td class="name" onclick="toggle('hasInitStatement1')"><a name="hasInitStatement1Anchor">hasInitStatement</a></td><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1Stmt.html";>Stmt</a>> InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="hasInitStatement1"><pre>Matches selection statements with initializer.
+
+Given:
+ void foo() {
+ if (int i = foobar(); i > 0) {}
+ switch (int i = foobar(); i) {}
+ for (auto& a = get_range(); auto& x : a) {}
+ }
+ void bar() {
+ if (foobar() > 0) {}
+ switch (foobar()) {}
+ for (auto& x : get_range()) {}
+ }
+ifStmt(hasInitStatement(anything()))
+ matches the if statement in foo but not in bar.
+switchStmt(hasInitStatement(anything()))
+ matches the switch statement in foo but not in bar.
+cxxForRangeStmt(hasInitStatement(anything()))
+ matches the range for statement in foo but not in bar.
+</pre></td></tr>
+
+
<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1TagType.html";>TagType</a>></td><td class="name" onclick="toggle('hasDeclaration4')"><a name="hasDeclaration4Anchor">hasDeclaration</a></td><td>const Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1Decl.html";>Decl</a>> InnerMatcher</td></tr>
<tr><td colspan="4" class="doc" id="hasDeclaration4"><pre>Matches a node if the declaration associated with that node
matches the given matcher.
@@ -7170,6 +7251,22 @@
</pre></td></tr>
+<tr><td>Matcher<T></td><td class="name" onclick="toggle('traverse0')"><a name="traverse0Anchor">traverse</a></td><td>ast_type_traits::TraversalKind TK, const Matcher<T> InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="traverse0"><pre>Causes all nested matchers to be matched with the specified traversal kind.
+
+Given
+ void foo()
+ {
+ int i = 3.0;
+ }
+The matcher
+ traverse(ast_type_traits::TK_IgnoreImplicitCastsAndParentheses,
+ varDecl(hasInitializer(floatLiteral().bind("init")))
+ )
+matches the variable declaration with "init" bound to the "3.0".
+</pre></td></tr>
+
+
<tr><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1TypedefNameDecl.html";>TypedefNameDecl</a>></td><td class="name" onclick="toggle('hasType2')"><a name="hasType2Anchor">hasType</a></td><td>Matcher<<a href="https://clang.llvm.org/doxygen/classclang_1_1QualType.html";>QualType</a>> InnerMatcher</td></tr>
<tr><td colspan="4" class="doc" id="hasType2"><pre>Matches if the expression's or declaration's type matches a type
matcher.
Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -fexceptions
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -fexceptions -std=c++17
namespace std {
struct string {
@@ -15,15 +15,15 @@
if (a > 0)
return;
else // comment-0
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return'
- // CHECK-FIXES: {{^}} // comment-0
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} // comment-0
return;
if (a > 0) {
return;
} else { // comment-1
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
- // CHECK-FIXES: {{^}} } // comment-1
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-1
return;
}
@@ -60,15 +60,15 @@
if (a < 10)
return;
else // comment-5
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
- // CHECK-FIXES: {{^}} // comment-5
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} // comment-5
f(0);
} else {
if (a > 10)
return;
else // comment-6
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
- // CHECK-FIXES: {{^}} // comment-6
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} // comment-6
f(0);
}
}
@@ -78,29 +78,29 @@
if (x) {
continue;
} else { // comment-7
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'continue'
- // CHECK-FIXES: {{^}} } // comment-7
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'continue'
+ // CHECK-FIXES: {{^}} } // comment-7
x++;
}
if (x) {
break;
} else { // comment-8
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'break'
- // CHECK-FIXES: {{^}} } // comment-8
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'break'
+ // CHECK-FIXES: {{^}} } // comment-8
x++;
}
if (x) {
throw 42;
} else { // comment-9
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw'
- // CHECK-FIXES: {{^}} } // comment-9
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw'
+ // CHECK-FIXES: {{^}} } // comment-9
x++;
}
if (x) {
throw my_exception("foo");
} else { // comment-10
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw'
- // CHECK-FIXES: {{^}} } // comment-10
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw'
+ // CHECK-FIXES: {{^}} } // comment-10
x++;
}
}
@@ -109,11 +109,68 @@
extern int *g();
extern void h(int **x);
-int *decl_in_condition() {
+int *declInConditionUsedInElse() {
if (int *x = g()) {
return x;
- } else {
+ } else { // comment-11
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-11
h(&x);
return x;
}
}
+int *declInConditionUnusedInElse() {
+ if (int *x = g()) {
+ h(&x);
+ return x;
+ } else { // comment-12
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-12
+ return nullptr;
+ }
+}
+
+int *varInitAndCondition() {
+ if (int *X = g(); X != nullptr) {
+ return X;
+ } else { // comment-13
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-13
+ h(&X);
+ return X;
+ }
+}
+
+int *varInitAndConditionUnusedInElse() {
+ if (int *X = g(); X != nullptr) {
+ return X;
+ } else { // comment-14
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-14
+ return nullptr;
+ }
+}
+
+int *initAndCondition() {
+ int *X;
+ if (X = g(); X != nullptr) {
+ return X;
+ } else { // comment-15
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-15
+ h(&X);
+ return X;
+ }
+}
+
+int *varInitAndConditionUnusedInElseWithDecl() {
+ int *Y = g();
+ if (int *X = g(); X != nullptr) {
+ return X;
+ } else { // comment-16
+ // CHECK-FIXES-NOT{{^}} } // comment-16
+ int *Y = g();
+ h(&Y);
+ }
+ return Y;
+}
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -23,39 +23,183 @@
breakStmt().bind("break"),
expr(ignoringImplicit(cxxThrowExpr().bind("throw")))));
Finder->addMatcher(
- compoundStmt(forEach(
- ifStmt(unless(isConstexpr()),
- // FIXME: Explore alternatives for the
- // `if (T x = ...) {... return; } else { <use x> }`
- // pattern:
- // * warn, but don't fix;
- // * fix by pulling out the variable declaration out of
- // the condition.
- unless(hasConditionVariableStatement(anything())),
- hasThen(stmt(anyOf(InterruptsControlFlow,
- compoundStmt(has(InterruptsControlFlow))))),
- hasElse(stmt().bind("else")))
- .bind("if"))),
+ compoundStmt(
+ forEach(ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
+ anyOf(InterruptsControlFlow,
+ compoundStmt(has(InterruptsControlFlow))))),
+ hasElse(stmt().bind("else")))
+ .bind("if")))
+ .bind("cs"),
this);
}
+const DeclRefExpr *findUsage(const Stmt *Node, int64_t DeclIdentifier) {
+ if (const auto *DeclRef = dyn_cast_or_null<DeclRefExpr>(Node)) {
+ if (DeclRef->getDecl()->getID() == DeclIdentifier) {
+ return DeclRef;
+ }
+ } else {
+ for (const auto &ChildNode : Node->children()) {
+ if (auto Result = findUsage(ChildNode, DeclIdentifier)) {
+ return Result;
+ }
+ }
+ }
+ return nullptr;
+}
+
+const DeclRefExpr *
+findUsageRange(const Stmt *Node,
+ const llvm::iterator_range<int64_t *> &DeclIdentifiers) {
+ if (const auto *DeclRef = dyn_cast_or_null<DeclRefExpr>(Node)) {
+ if (llvm::is_contained(DeclIdentifiers, DeclRef->getDecl()->getID())) {
+ return DeclRef;
+ }
+ } else {
+ for (const auto &ChildNode : Node->children()) {
+ if (auto Result = findUsageRange(ChildNode, DeclIdentifiers)) {
+ return Result;
+ }
+ }
+ }
+ return nullptr;
+}
+
+const DeclRefExpr *checkInitDeclUsageInElse(const IfStmt *If) {
+ auto InitDeclStmt = dyn_cast_or_null<DeclStmt>(If->getInit());
+ if (!InitDeclStmt)
+ return nullptr;
+ if (InitDeclStmt->isSingleDecl()) {
+ auto InitDecl = InitDeclStmt->getSingleDecl();
+ assert(isa<VarDecl>(InitDecl) && "SingleDecl must be a VarDecl");
+ return findUsage(If->getElse(), InitDecl->getID());
+ }
+ llvm::SmallVector<int64_t, 4> DeclIdentifiers;
+ for (const auto &Decl : InitDeclStmt->decls()) {
+ assert(isa<VarDecl>(Decl) && "Init Decls must be a VarDecl");
+ DeclIdentifiers.push_back(Decl->getID());
+ }
+ return findUsageRange(If->getElse(), DeclIdentifiers);
+}
+
+const DeclRefExpr *checkConditionVarUsageInElse(const IfStmt *If) {
+ auto CondVar = If->getConditionVariable();
+ return CondVar != nullptr ? findUsage(If->getElse(), CondVar->getID())
+ : nullptr;
+}
+
+bool containsDeclInScope(const Stmt *Node) {
+ if (isa<DeclStmt>(Node)) {
+ return true;
+ }
+ if (auto Compound = dyn_cast<CompoundStmt>(Node)) {
+ return llvm::any_of(Compound->body(), [](const Stmt *SubNode) {
+ return isa<DeclStmt>(SubNode);
+ });
+ }
+ return false;
+}
+
+void removeElseAndBrackets(DiagnosticBuilder &Diag, ASTContext &Context,
+ const Stmt *Else, SourceLocation ElseLoc) {
+ auto Remap = [&](SourceLocation Loc) {
+ return Context.getSourceManager().getExpansionLoc(Loc);
+ };
+ auto TokLen = [&](SourceLocation Loc) {
+ return Lexer::MeasureTokenLength(Loc, Context.getSourceManager(),
+ Context.getLangOpts());
+ };
+
+ if (auto CS = dyn_cast<CompoundStmt>(Else)) {
+ Diag << tooling::fixit::createRemoval(ElseLoc);
+ const auto LBrace = CS->getLBracLoc();
+ const auto RBrace = CS->getRBracLoc();
+ const auto RangeStart = Remap(LBrace).getLocWithOffset(TokLen(LBrace));
+ const auto RangeEnd = Remap(RBrace).getLocWithOffset(-1);
+
+ const auto Repl = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(RangeStart, RangeEnd),
+ Context.getSourceManager(), Context.getLangOpts());
+ Diag << tooling::fixit::createReplacement(CS->getSourceRange(), Repl);
+ } else {
+ const auto ElseExpandedLoc = Remap(ElseLoc);
+ const auto EndLoc = Remap(Else->getEndLoc());
+
+ const auto Repl = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(
+ ElseExpandedLoc.getLocWithOffset(TokLen(ElseLoc)), EndLoc),
+ Context.getSourceManager(), Context.getLangOpts());
+ Diag << tooling::fixit::createReplacement(
+ SourceRange(ElseExpandedLoc, EndLoc), Repl);
+ }
+}
+
void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
+ const auto If = Result.Nodes.getNodeAs<IfStmt>("if");
+ const auto Else = Result.Nodes.getNodeAs<Stmt>("else");
+ const auto OuterScope = Result.Nodes.getNodeAs<CompoundStmt>("cs");
+ const bool IsLastInScope = OuterScope->body_back() == If;
+
+ if (!IsLastInScope && containsDeclInScope(Else)) {
+ // {
+ // ...
+ // if (a) { return }
+ // else { int b = 0; } cant remove the else as b will leak its scope
+ // ...
+ // }
+ return;
+ }
+
SourceLocation ElseLoc = If->getElseLoc();
- std::string ControlFlowInterruptor;
- for (const auto *BindingName : {"return", "continue", "break", "throw"})
- if (Result.Nodes.getNodeAs<Stmt>(BindingName))
- ControlFlowInterruptor = BindingName;
+ llvm::StringRef ControlFlowInterruptor = [&]() -> llvm::StringRef {
+ for (llvm::StringRef BindingName : {"return", "continue", "break", "throw"})
+ if (Result.Nodes.getNodeAs<Stmt>(BindingName))
+ return BindingName;
+ return {};
+ }();
+ if (checkInitDeclUsageInElse(If) != nullptr ||
+ checkConditionVarUsageInElse(If) != nullptr) {
+ if (IsLastInScope) {
+ // If the if statement is the last statement its enclosing statements
+ // scope, we can pull the decl out of the if statement.
+ DiagnosticBuilder Diag = diag(ElseLoc, "do not use 'else' after '%0'",
+ clang::DiagnosticIDs::Level::Remark)
+ << ControlFlowInterruptor;
+ if (If->hasInitStorage()) {
+ Diag << tooling::fixit::createReplacement(
+ SourceRange(If->getIfLoc()),
+ (tooling::fixit::getText(*If->getInit(), *Result.Context) +
+ llvm::StringRef("\n") +
+ tooling::fixit::getText(If->getIfLoc(), *Result.Context))
+ .str())
+ << tooling::fixit::createRemoval(If->getInit()->getSourceRange());
+ removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
+ } else { // condition variable
+ auto VDeclStmt = If->getConditionVariableDeclStmt();
+ auto VDecl = If->getConditionVariable();
+ auto Repl = (tooling::fixit::getText(*VDeclStmt, *Result.Context) +
+ llvm::StringRef(";\n") +
+ tooling::fixit::getText(If->getIfLoc(), *Result.Context))
+ .str();
+ Diag << tooling::fixit::createReplacement(SourceRange(If->getIfLoc()),
+ Repl)
+ << tooling::fixit::createReplacement(VDeclStmt->getSourceRange(),
+ VDecl->getName());
+ removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
+ }
+ } else {
+ // warn, but don't attempt an autofix
+ diag(ElseLoc, "do not use 'else' after '%0'",
+ clang::DiagnosticIDs::Level::Remark)
+ << ControlFlowInterruptor;
+ }
+ return;
+ }
DiagnosticBuilder Diag = diag(ElseLoc, "do not use 'else' after '%0'")
<< ControlFlowInterruptor;
- Diag << tooling::fixit::createRemoval(ElseLoc);
-
- // FIXME: Removing the braces isn't always safe. Do a more careful analysis.
- // FIXME: Change clang-format to correctly un-indent the code.
- if (const auto *CS = Result.Nodes.getNodeAs<CompoundStmt>("else"))
- Diag << tooling::fixit::createRemoval(CS->getLBracLoc())
- << tooling::fixit::createRemoval(CS->getRBracLoc());
+ removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
}
} // namespace readability
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
