[clang-tools-extra] r278257 - [clang-tidy] enhance readability-else-after-return

[Kirill Bobyrev via cfe-commits]

Author: omtcyfz
Date: Wed Aug 10 13:05:47 2016
New Revision: 278257

URL: http://llvm.org/viewvc/llvm-project?rev=278257&view=rev
Log:
[clang-tidy] enhance readability-else-after-return

`readability-else-after-return` only warns about `return` calls, but LLVM Coding
Standars stat that `throw`, `continue`, `goto`, etc after `return` calls are
bad, too.

Reviwers: alexfh, aaron.ballman

Differential Revision: https://reviews.llvm.org/D23265

Modified:
    clang-tools-extra/trunk/clang-tidy/readability/ElseAfterReturnCheck.cpp
    clang-tools-extra/trunk/docs/clang-tidy/checks/readability-else-after-return.rst
    clang-tools-extra/trunk/test/clang-tidy/readability-else-after-return.cpp

Modified: clang-tools-extra/trunk/clang-tidy/readability/ElseAfterReturnCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/ElseAfterReturnCheck.cpp?rev=278257&r1=278256&r2=278257&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/readability/ElseAfterReturnCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/readability/ElseAfterReturnCheck.cpp Wed Aug 10 13:05:47 2016
@@ -19,20 +19,29 @@ namespace tidy {
 namespace readability {
 
 void ElseAfterReturnCheck::registerMatchers(MatchFinder *Finder) {
-  // FIXME: Support continue, break and throw.
+  const auto ControlFlowInterruptorMatcher =
+      stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"),
+                 breakStmt().bind("break"), cxxThrowExpr().bind("throw")));
   Finder->addMatcher(
-      compoundStmt(
-          forEach(ifStmt(hasThen(stmt(anyOf(returnStmt(),
-                                            compoundStmt(has(returnStmt()))))),
-                         hasElse(stmt().bind("else")))
-                      .bind("if"))),
+      stmt(forEach(
+          ifStmt(hasThen(stmt(
+                     anyOf(ControlFlowInterruptorMatcher,
+                           compoundStmt(has(ControlFlowInterruptorMatcher))))),
+                 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", "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);
 
   // FIXME: Removing the braces isn't always safe. Do a more careful analysis.

Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/readability-else-after-return.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/readability-else-after-return.rst?rev=278257&r1=278256&r2=278257&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/readability-else-after-return.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/readability-else-after-return.rst Wed Aug 10 13:05:47 2016
@@ -3,7 +3,62 @@
 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 enforcements of that. Please do not use
+`else` or `else if` after something that interrupts control flow - like
+`return`, `break`, `continue`, `throw`.
 
-Flags the usages of ``else`` after ``return``.
+The following piece of code illustrates how the check works. This piece of code:
 
-http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
+.. code-block:: 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++;
+        }
+      }
+    }
+
+
+Would be transformed into:
+
+.. code-block:: 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++;
+      }
+    }
+
+
+This checks helps to enforce this `Coding Standars recommendation
+<http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return>`_.

Modified: clang-tools-extra/trunk/test/clang-tidy/readability-else-after-return.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-else-after-return.cpp?rev=278257&r1=278256&r2=278257&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/readability-else-after-return.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/readability-else-after-return.cpp Wed Aug 10 13:05:47 2016
@@ -3,16 +3,16 @@
 void f(int a) {
   if (a > 0)
     return;
-  else // comment
-// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: don't use else after return
-// CHECK-FIXES: {{^}}  // comment
+  else // 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
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: don't use else after return
-// CHECK-FIXES:  } // comment
+  } else { // comment-1
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+  // CHECK-FIXES: {{^}}  } // comment-1
     return;
   }
 
@@ -28,7 +28,34 @@ void f(int a) {
     f(0);
   else if (a > 10)
     return;
-  else
+  else // comment-2
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use 'else' after 'return'
+  // CHECK-FIXES: {{^}}  // comment-2
     f(0);
 }
 
+void foo() {
+  for (unsigned x = 0; x < 42; ++x) {
+    if (x) {
+      continue;
+    } else { // comment-3
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'continue'
+    // CHECK-FIXES: {{^}}    } // comment-3
+      x++;
+    }
+    if (x) {
+      break;
+    } else { // comment-4
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'break'
+    // CHECK-FIXES: {{^}}    } // comment-4
+      x++;
+    }
+    if (x) {
+      throw 42;
+    } else { // comment-5
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not use 'else' after 'throw'
+    // CHECK-FIXES: {{^}}    } // comment-5
+      x++;
+    }
+  }
+}