[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE344785: [clang-tidy] Resolve readability-else-after-return
false positive for constexpr… (authored by mkurdej, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D53372?vs=170152=170201#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53372
Files:
clang-tidy/readability/ElseAfterReturnCheck.cpp
test/clang-tidy/readability-else-after-return-if-constexpr.cpp
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -1,57 +1,58 @@
-//===--- ElseAfterReturnCheck.cpp - clang-tidy-===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===--===//
-
-#include "ElseAfterReturnCheck.h"
-#include "clang/AST/ASTContext.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Tooling/FixIt.h"
-
-using namespace clang::ast_matchers;
-
-namespace clang {
-namespace tidy {
-namespace readability {
-
-void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
- const auto ControlFlowInterruptorMatcher =
- stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"),
- breakStmt().bind("break"),
- expr(ignoringImplicit(cxxThrowExpr().bind("throw");
- Finder->addMatcher(
- compoundStmt(forEach(
- ifStmt(hasThen(stmt(
- anyOf(ControlFlowInterruptorMatcher,
- compoundStmt(has(ControlFlowInterruptorMatcher),
- hasElse(stmt().bind("else")))
- .bind("if"))),
- this);
-}
-
-void ElseAfterReturnCheck::check(const MatchFinder::MatchResult ) {
- const auto *If = Result.Nodes.getNodeAs("if");
- SourceLocation ElseLoc = If->getElseLoc();
- std::string ControlFlowInterruptor;
- for (const auto *BindingName : {"return", "continue", "break", "throw"})
-if (Result.Nodes.getNodeAs(BindingName))
- ControlFlowInterruptor = BindingName;
-
- 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("else"))
-Diag << tooling::fixit::createRemoval(CS->getLBracLoc())
- << tooling::fixit::createRemoval(CS->getRBracLoc());
-}
-
-} // namespace readability
-} // namespace tidy
-} // namespace clang
+//===--- ElseAfterReturnCheck.cpp - clang-tidy-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ElseAfterReturnCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
+ const auto ControlFlowInterruptorMatcher =
+ stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"),
+ breakStmt().bind("break"),
+ expr(ignoringImplicit(cxxThrowExpr().bind("throw");
+ Finder->addMatcher(
+ compoundStmt(forEach(
+ ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
+ anyOf(ControlFlowInterruptorMatcher,
+ compoundStmt(has(ControlFlowInterruptorMatcher),
+ hasElse(stmt().bind("else")))
+ .bind("if"))),
+ this);
+}
+
+void ElseAfterReturnCheck::check(const MatchFinder::MatchResult ) {
+ const auto *If = Result.Nodes.getNodeAs("if");
+ SourceLocation ElseLoc = If->getElseLoc();
+ std::string ControlFlowInterruptor;
+ for (const auto *BindingName : {"return", "continue", "break", "throw"})
+if (Result.Nodes.getNodeAs(BindingName))
+ ControlFlowInterruptor = BindingName;
+
+ 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("else"))
+Diag <<
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
curdeius updated this revision to Diff 170152.
curdeius added a comment.
Fixed diff.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53372
Files:
clang-tidy/readability/ElseAfterReturnCheck.cpp
test/clang-tidy/readability-else-after-return-if-constexpr.cpp
Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+ if (sizeof(int) > 4)
+return;
+ else
+return;
+ // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not use 'else' after 'return'
+
+ if constexpr (sizeof(int) > 4)
+return;
+ else
+return;
+
+ if constexpr (sizeof(int) > 4)
+return;
+ else if constexpr (sizeof(long) > 4)
+return;
+ else
+return;
+}
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
expr(ignoringImplicit(cxxThrowExpr().bind("throw");
Finder->addMatcher(
compoundStmt(forEach(
- ifStmt(hasThen(stmt(
+ ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
hasElse(stmt().bind("else")))
Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+ if (sizeof(int) > 4)
+return;
+ else
+return;
+ // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not use 'else' after 'return'
+
+ if constexpr (sizeof(int) > 4)
+return;
+ else
+return;
+
+ if constexpr (sizeof(int) > 4)
+return;
+ else if constexpr (sizeof(long) > 4)
+return;
+ else
+return;
+}
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
expr(ignoringImplicit(cxxThrowExpr().bind("throw");
Finder->addMatcher(
compoundStmt(forEach(
- ifStmt(hasThen(stmt(
+ ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
hasElse(stmt().bind("else")))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
aaron.ballman added a comment. Hmm, the latest patch only seems to have the changes to the test but not the implementation? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
curdeius updated this revision to Diff 170101. curdeius added a comment. Applied changes as per comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53372 Files: test/clang-tidy/readability-else-after-return-if-constexpr.cpp Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp === --- test/clang-tidy/readability-else-after-return-if-constexpr.cpp +++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp @@ -6,7 +6,7 @@ return; else return; - // CHECK-MESSAGES: [[@LINE-2]]:3: warning: + // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not use 'else' after 'return' if constexpr (sizeof(int) > 4) return; @@ -20,4 +20,3 @@ else return; } -// CHECK-NOT: warning: Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp === --- test/clang-tidy/readability-else-after-return-if-constexpr.cpp +++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp @@ -6,7 +6,7 @@ return; else return; - // CHECK-MESSAGES: [[@LINE-2]]:3: warning: + // CHECK-MESSAGES: [[@LINE-2]]:3: warning: do not use 'else' after 'return' if constexpr (sizeof(int) > 4) return; @@ -20,4 +20,3 @@ else return; } -// CHECK-NOT: warning: ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
aaron.ballman added inline comments. Comment at: test/clang-tidy/readability-else-after-return-if-constexpr.cpp:9 +return; + // CHECK-MESSAGES: [[@LINE-2]]:3: warning: + Please add some of the warning text -- any warning will match this. Comment at: test/clang-tidy/readability-else-after-return-if-constexpr.cpp:23 +} +// CHECK-NOT: warning: This seems unnecessary. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
lebedev.ri added a comment. In https://reviews.llvm.org/D53372#1268207, @curdeius wrote: > In https://reviews.llvm.org/D53372#1267728, @lebedev.ri wrote: > > > I think it would be good to add some more explanation as to *why* that > > `else` has to be kept. > > > Do you mean add a comment in the code or just an explanation for the review? > In this case removing else will just provoke a compilation error. There may > be some cases where you may remove else though. I was indeed talking about the review, thank you. In https://reviews.llvm.org/D53372#1268208, @curdeius wrote: > Actually, I can make it an option for this check to skip or not constexpr > ifs, WDYT? I'm not sure, maybe this shouldn't be an option. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
curdeius added a comment. Actually, I can make it an option for this check to skip or not constexpr ifs, WDYT? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
curdeius added a comment.
In https://reviews.llvm.org/D53372#1267728, @lebedev.ri wrote:
> I think it would be good to add some more explanation as to *why* that `else`
> has to be kept.
Do you mean add a comment in the code or just an explanation for the review?
For the latter, e.g.:
// unrelated types:
struct type_a {};
struct type_b {};
auto f()
{
if constexpr(condition)
{
return type_a{};
}
else
{
return type_b{};
}
}
In this case removing else will just provoke a compilation error. There may be
some cases where you may remove else though.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53372
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
lebedev.ri added a comment. I think it would be good to add some more explanation as to *why* that `else` has to be kept. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53372 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.
curdeius created this revision.
curdeius added reviewers: alexfh, aaron.ballman.
Herald added subscribers: cfe-commits, xazax.hun.
It fixes the false positive when using constexpr if and where else cannot be
removed:
Example:
if constexpr (sizeof(int) > 4)
// ...
return /* ... */;
else // This else cannot be removed.
// ...
return /* ... */;
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53372
Files:
clang-tidy/readability/ElseAfterReturnCheck.cpp
test/clang-tidy/readability-else-after-return-if-constexpr.cpp
Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+ if (sizeof(int) > 4)
+return;
+ else
+return;
+ // CHECK-MESSAGES: [[@LINE-2]]:3: warning:
+
+ if constexpr (sizeof(int) > 4)
+return;
+ else
+return;
+
+ if constexpr (sizeof(int) > 4)
+return;
+ else if constexpr (sizeof(long) > 4)
+return;
+ else
+return;
+}
+// CHECK-NOT: warning:
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
expr(ignoringImplicit(cxxThrowExpr().bind("throw");
Finder->addMatcher(
compoundStmt(forEach(
- ifStmt(hasThen(stmt(
+ ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
hasElse(stmt().bind("else")))
Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+ if (sizeof(int) > 4)
+return;
+ else
+return;
+ // CHECK-MESSAGES: [[@LINE-2]]:3: warning:
+
+ if constexpr (sizeof(int) > 4)
+return;
+ else
+return;
+
+ if constexpr (sizeof(int) > 4)
+return;
+ else if constexpr (sizeof(long) > 4)
+return;
+ else
+return;
+}
+// CHECK-NOT: warning:
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
expr(ignoringImplicit(cxxThrowExpr().bind("throw");
Finder->addMatcher(
compoundStmt(forEach(
- ifStmt(hasThen(stmt(
+ ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
hasElse(stmt().bind("else")))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
