njames93 updated this revision to Diff 235752.
njames93 added a comment.
hopefully adhered to all conventions :)
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 {
@@ -106,14 +106,109 @@
}
}
-extern int *g();
-extern void h(int **x);
+int g();
+int h(int);
-int *decl_in_condition() {
- if (int *x = g()) {
- return x;
+int declInConditionUsedInElse() {
+ if (int X = g()) { // comment-11
+ // CHECK-FIXES: {{^}} int X = g();
+ // CHECK-FIXES-NEXT: {{^}}if (X) { // comment-11
+ return X;
+ } else { // comment-11
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-11
+ return h(X);
+ }
+}
+int declInConditionUnusedInElse() {
+ if (int X = g()) {
+ return h(X);
+ } else { // comment-12
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-12
+ return 0;
+ }
+}
+
+int varInitAndCondition() {
+ if (int X = g(); X != 0) { // comment-13
+ // CHECK-FIXES: {{^}} int X = g();
+ // CHECK-FIXES-NEXT: {{^}}if ( X != 0) { // comment-13
+ return X;
+ } else { // comment-13
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-13
+ return h(X);
+ }
+}
+
+int varInitAndConditionUnusedInElse() {
+ if (int X = g(); X != 0) {
+ return X;
+ } else { // comment-14
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-14
+ return 0;
+ }
+}
+
+int initAndCondition() {
+ int X;
+ if (X = g(); X != 0) {
+ return X;
+ } else { // comment-15
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-15
+ return h(X);
+ }
+}
+
+int varInitAndConditionUnusedInElseWithDecl() {
+ int Y = g();
+ if (int X = g(); X != 0) {
+ return X;
+ } else { // comment-16
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES-NOT: {{^}} } //comment-16
+ int Y = g();
+ h(Y);
+ }
+ return Y;
+}
+
+int varInitAndCondVarUsedInElse() {
+ if (int X = g(); int Y = g()) { // comment-17
+ // CHECK-FIXES: {{^}} int X = g();
+ // CHECK-FIXES-NEXT: {{^}}int Y = g();
+ // CHECK-FIXES-NEXT: {{^}}if ( Y) { // comment-17
+ return X ? X : Y;
+ } else { // comment-17
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-17
+ return X ? X : h(Y);
+ }
+}
+
+int lifeTimeExtensionTests(int a) {
+ if (a > 0) {
+ return a;
+ } else {
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ int b = 0;
+ }
+ if (int b = a; b & 1 == 0) {
+ return a;
} else {
- h(&x);
- return x;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ b++;
+ }
+ if (int b = a; b > 1) { // comment-18
+ // CHECK-FIXES: {{^}} int b = a;
+ // CHECK-FIXES-NEXT: {{^}}if ( b > 1) { // comment-18
+ return a;
+ } else { // comment-18
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: {{^}} } // comment-18
+ return b;
}
}
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
@@ -10,6 +10,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Tooling/FixIt.h"
+#include "llvm/ADT/SmallVector.h"
using namespace clang::ast_matchers;
@@ -17,45 +18,212 @@
namespace tidy {
namespace readability {
+static const char ReturnStr[] = "return";
+static const char ContinueStr[] = "continue";
+static const char BreakStr[] = "break";
+static const char ThrowStr[] = "throw";
+static const char WarningMessage[] = "do not use 'else' after '%0'";
+
void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
const auto InterruptsControlFlow =
- stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"),
- breakStmt().bind("break"),
- expr(ignoringImplicit(cxxThrowExpr().bind("throw")))));
+ stmt(anyOf(returnStmt().bind(ReturnStr), continueStmt().bind(ContinueStr),
+ breakStmt().bind(BreakStr),
+ expr(ignoringImplicit(cxxThrowExpr().bind(ThrowStr)))));
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<DeclRefExpr>(Node)) {
+ if (DeclRef->getDecl()->getID() == DeclIdentifier) {
+ return DeclRef;
+ }
+ } else {
+ for (const Stmt *ChildNode : Node->children()) {
+ if (const DeclRefExpr *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<DeclRefExpr>(Node)) {
+ if (llvm::is_contained(DeclIdentifiers, DeclRef->getDecl()->getID())) {
+ return DeclRef;
+ }
+ } else {
+ for (const Stmt *ChildNode : Node->children()) {
+ if (const DeclRefExpr *Result =
+ findUsageRange(ChildNode, DeclIdentifiers)) {
+ return Result;
+ }
+ }
+ }
+ return nullptr;
+}
+
+const DeclRefExpr *checkInitDeclUsageInElse(const IfStmt *If) {
+ const auto *InitDeclStmt = dyn_cast_or_null<DeclStmt>(If->getInit());
+ if (!InitDeclStmt)
+ return nullptr;
+ if (InitDeclStmt->isSingleDecl()) {
+ const Decl *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 Decl *ChildDecl : InitDeclStmt->decls()) {
+ assert(isa<VarDecl>(ChildDecl) && "Init Decls must be a VarDecl");
+ DeclIdentifiers.push_back(ChildDecl->getID());
+ }
+ return findUsageRange(If->getElse(), DeclIdentifiers);
+}
+
+const DeclRefExpr *checkConditionVarUsageInElse(const IfStmt *If) {
+ const VarDecl *CondVar = If->getConditionVariable();
+ return CondVar != nullptr ? findUsage(If->getElse(), CondVar->getID())
+ : nullptr;
+}
+
+bool containsDeclInScope(const Stmt *Node) {
+ if (isa<DeclStmt>(Node)) {
+ return true;
+ }
+ if (const 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 (const auto *CS = dyn_cast<CompoundStmt>(Else)) {
+ Diag << tooling::fixit::createRemoval(ElseLoc);
+ SourceLocation LBrace = CS->getLBracLoc();
+ SourceLocation RBrace = CS->getRBracLoc();
+ SourceLocation RangeStart =
+ Remap(LBrace).getLocWithOffset(TokLen(LBrace) + 1);
+ SourceLocation RangeEnd = Remap(RBrace).getLocWithOffset(-1);
+
+ llvm::StringRef Repl = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(RangeStart, RangeEnd),
+ Context.getSourceManager(), Context.getLangOpts());
+ Diag << tooling::fixit::createReplacement(CS->getSourceRange(), Repl);
+ } else {
+ SourceLocation ElseExpandedLoc = Remap(ElseLoc);
+ SourceLocation EndLoc = Remap(Else->getEndLoc());
+
+ llvm::StringRef Repl = Lexer::getSourceText(
+ CharSourceRange::getTokenRange(
+ ElseExpandedLoc.getLocWithOffset(TokLen(ElseLoc) + 1), 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 *Else = Result.Nodes.getNodeAs<Stmt>("else");
+ const auto *OuterScope = Result.Nodes.getNodeAs<CompoundStmt>("cs");
+
+ bool IsLastInScope = OuterScope->body_back() == If;
SourceLocation ElseLoc = If->getElseLoc();
- std::string ControlFlowInterruptor;
- for (const auto *BindingName : {"return", "continue", "break", "throw"})
- if (Result.Nodes.getNodeAs<Stmt>(BindingName))
- ControlFlowInterruptor = BindingName;
- DiagnosticBuilder Diag = diag(ElseLoc, "do not use 'else' after '%0'")
- << ControlFlowInterruptor;
- Diag << tooling::fixit::createRemoval(ElseLoc);
+ auto ControlFlowInterruptor = [&]() -> llvm::StringRef {
+ for (llvm::StringRef BindingName :
+ {ReturnStr, ContinueStr, BreakStr, ThrowStr})
+ if (Result.Nodes.getNodeAs<Stmt>(BindingName))
+ return BindingName;
+ return {};
+ }();
+
+ if (!IsLastInScope && containsDeclInScope(Else)) {
+ // Warn, but don't attempt an autofix.
+ diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
+ return;
+ }
+
+ if (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, WarningMessage, clang::DiagnosticIDs::Level::Remark)
+ << ControlFlowInterruptor;
+ if (checkInitDeclUsageInElse(If) != nullptr) {
+ Diag << tooling::fixit::createReplacement(
+ SourceRange(If->getIfLoc()),
+ (tooling::fixit::getText(*If->getInit(), *Result.Context) +
+ llvm::StringRef("\n"))
+ .str())
+ << tooling::fixit::createRemoval(If->getInit()->getSourceRange());
+ }
+ const DeclStmt *VDeclStmt = If->getConditionVariableDeclStmt();
+ const VarDecl *VDecl = If->getConditionVariable();
+ std::string 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, WarningMessage) << ControlFlowInterruptor;
+ }
+ return;
+ }
- // 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());
+ if (checkInitDeclUsageInElse(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, WarningMessage)
+ << ControlFlowInterruptor;
+ Diag << tooling::fixit::createReplacement(
+ SourceRange(If->getIfLoc()),
+ (tooling::fixit::getText(*If->getInit(), *Result.Context) +
+ "\n" +
+ tooling::fixit::getText(If->getIfLoc(), *Result.Context))
+ .str())
+ << tooling::fixit::createRemoval(If->getInit()->getSourceRange());
+ removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
+ } else {
+ // Warn, but don't attempt an autofix.
+ diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
+ }
+ return;
+ }
+
+ DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
+ << ControlFlowInterruptor;
+ 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
