omtcyfz updated this revision to Diff 67347.
omtcyfz marked 2 inline comments as done.
omtcyfz added a comment.
Address the comments.
https://reviews.llvm.org/D23265
Files:
clang-tidy/readability/ElseAfterReturnCheck.cpp
docs/clang-tidy/checks/readability-else-after-return.rst
test/clang-tidy/readability-else-after-return.cpp
Index: test/clang-tidy/readability-else-after-return.cpp
===================================================================
--- test/clang-tidy/readability-else-after-return.cpp
+++ test/clang-tidy/readability-else-after-return.cpp
@@ -4,15 +4,15 @@
if (a > 0)
return;
else // comment
-// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: don't use else after return
-// CHECK-FIXES: {{^}} // comment
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: // comment
return;
if (a > 0) {
return;
} else { // comment
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: don't use else after return
-// CHECK-FIXES: } // comment
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: // comment
return;
}
@@ -28,7 +28,42 @@
f(0);
else if (a > 10)
return;
- else
+ else // comment
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return'
+ // CHECK-FIXES: // comment
f(0);
}
+void foo() {
+label:
+ for (unsigned x = 0; x < 42; ++x) {
+ if (x) {
+ continue;
+ } else { // comment
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'continue'
+ // CHECK-FIXES: // comment
+ x++;
+ }
+ if (x) {
+ break;
+ } else { // comment
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'break'
+ // CHECK-FIXES: // comment
+ x++;
+ }
+ if (x) {
+ throw 42;
+ } else { // comment
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw'
+ // CHECK-FIXES: // comment
+ x++;
+ }
+ if (x) {
+ goto label;
+ } else { // comment
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'goto'
+ // CHECK-FIXES: // comment
+ x++;
+ }
+ }
+}
Index: docs/clang-tidy/checks/readability-else-after-return.rst
===================================================================
--- docs/clang-tidy/checks/readability-else-after-return.rst
+++ docs/clang-tidy/checks/readability-else-after-return.rst
@@ -3,7 +3,72 @@
readability-else-after-return
=============================
+`LLVM Coding Standards <http://llvm.org/docs/CodingStandards.html>`_ advises to
+reduce indentation where possible and where it makes understanding code easier.
+Early exit is one of the suggested enforcement of that. Please do not use `else`
+or `else if` after something that interrupts control flow - like `return`,
+`break`, `continue`, `throw`, `goto`, etc.
-Flags the usages of ``else`` after ``return``.
+Following piece of code illustrates how check would work. This piece of code:
-http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
+``` c++
+void foo(int Value) {
+ int Local = 0;
+ for (int i = 0; i < 42; i++) {
+ if (Value == 1) {
+ return;
+ } else {
+ Local++;
+ }
+
+ if (Value == 2)
+ continue;
+ else
+ Local++;
+
+ if (Value == 3) {
+ throw 42;
+ } else {
+ Local++;
+ }
+
+ if (Value == 4) {
+ goto label;
+ } else {
+ Local++;
+ }
+ }
+}
+```
+
+Would be transformed into:
+
+``` c++
+void foo(int Value) {
+ int Local = 0;
+ for (int i = 0; i < 42; i++) {
+ if (Value == 1) {
+ return;
+ }
+ Local++;
+
+ if (Value == 2)
+ continue;
+ Local++;
+
+ if (Value == 3) {
+ throw 42;
+ }
+ Local++;
+
+ if (Value == 4) {
+ goto label;
+ }
+ Local++;
+ }
+}
+```
+
+
+This checks helps to enforce this `Coding Standars recommendation
+<http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return>`_.
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===================================================================
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -19,20 +19,37 @@
namespace readability {
void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
- // FIXME: Support continue, break and throw.
Finder->addMatcher(
- compoundStmt(
- forEach(ifStmt(hasThen(stmt(anyOf(returnStmt(),
- compoundStmt(has(returnStmt()))))),
+ stmt(
+ forEach(ifStmt(hasThen(stmt(anyOf(
+ returnStmt().bind("return"),
+ compoundStmt(has(returnStmt().bind("return"))),
+ continueStmt().bind("continue"),
+ compoundStmt(has(continueStmt().bind("continue"))),
+ breakStmt().bind("break"),
+ compoundStmt(has(breakStmt().bind("break"))),
+ gotoStmt().bind("goto"),
+ compoundStmt(has(gotoStmt().bind("goto"))),
+ cxxThrowExpr().bind("throw"),
+ compoundStmt(has(cxxThrowExpr().bind("throw")))))),
hasElse(stmt().bind("else")))
.bind("if"))),
this);
}
void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
SourceLocation ElseLoc = If->getElseLoc();
- DiagnosticBuilder Diag = diag(ElseLoc, "don't use else after return");
+ std::string ControlFlowInterruptor;
+ for (const auto &BindingName :
+ {"return", "continue", "break", "goto", "throw"}) {
+ if (Result.Nodes.getNodeAs<Stmt>(BindingName)) {
+ ControlFlowInterruptor = BindingName;
+ }
+ }
+
+ DiagnosticBuilder Diag = diag(ElseLoc, "do not use 'else' after '" +
+ ControlFlowInterruptor + '\'');
Diag << tooling::fixit::createRemoval(ElseLoc);
// FIXME: Removing the braces isn't always safe. Do a more careful analysis.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
