[clang-tidy] enhance readability-else-after-return

[omtcyfz]

Summary

readability-else-after-return only warns about return calls, but Coding Stadnards mention throw, continue, goto, etc.

[aaron.ballman]

clang-tidy/readability/ElseAfterReturnCheck.cpp

38–40 ↗

(On Diff #67160)

I think it might be better to specifically identify the flow control construct used instead of a laundry list of possibilities. Then it can be "do not use '%select{else|else if}0' after '%1'".

[alexfh]

clang-tidy/readability/ElseAfterReturnCheck.cpp

38–40 ↗

(On Diff #67160)

The wording is actually good. For documentation (currently, http://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html is not extremely verbose ;)).

Otherwise, I agree with Aaron: the warning should be precise to avoid confusion.

Also, no need for a separate variable to keep the message, IMO.

[omtcyfz]

Address the comments.

[alexfh]

clang-tidy/readability/ElseAfterReturnCheck.cpp

25 ↗

(On Diff #67347)

Let's structure this differently to reduce code duplication:

auto returnLikeStmt = stmt(anyOf(returnStmt().bind("return"), continueStmt().bind("continue"), ...));
Finder->addMatcher(stmt(forEach(ifStmt(hasThen(anyOf(returnLikeStmt, compoundStmt(has(returnLikeStmt)))), hasElse(...

[alexfh]

clang-tidy/readability/ElseAfterReturnCheck.cpp

45 ↗

(On Diff #67347)

This won't work in MSVC2013, I think. Just add a const char *Labels[] = {"return", ... (add more consts, constexprs or statics, if you like ;)

51 ↗

(On Diff #67347)

Please use diagnostic formatting facilities instead of string concatenation:

diag(L, "xxx '%0' yyy") << ControlFlowInterruptor;

docs/clang-tidy/checks/readability-else-after-return.rst

10 ↗

(On Diff #67347)

I would omit goto, since it's unclear, where the target label is (and it's not a common construct anyway).

test/clang-tidy/readability-else-after-return.cpp

33 ↗

(On Diff #67347)

CHECK-FIXES pattern should be sticter. Current pattern will match for any // comment occurrence anywhere in the test file, even if it's on a different line and still prefixed with else.

Two action items here: 1. make comments and patterns unique, 2. anchor patterns to the start of line ({{ˆ}}).

[omtcyfz]

clang-tidy/readability/ElseAfterReturnCheck.cpp

45 ↗

(On Diff #67347)

Hm, I'm confused. What exactly wouldn't work in MSVC?

test/clang-tidy/readability-else-after-return.cpp

33 ↗

(On Diff #67347)

Why make comments and patterns unique? If I anchor patterns to the start of line, it will only look at the line above, why does it need to be unique then?

[aaron.ballman]

clang-tidy/readability/ElseAfterReturnCheck.cpp

45 ↗

(On Diff #67347)

This one *will* work in MSVC 2013 (it was a different initializer list that @alexfh was thinking of, perhaps, where the deduced type of the list was a bit more complex). I just tried it out and the above construct compiles fine with MSVC.

That being said, const auto & is the wrong type here, it should be const auto *.

[omtcyfz]

clang-tidy/readability/ElseAfterReturnCheck.cpp

45 ↗

(On Diff #67347)

Hm, I'm confused. Why should it be a pointer?

So, when I have something like:

std::vector<unsigned> vec = {1, 2, 3};
for (const auto &x : vec) ...

This is right, I guess. Why is that different with init list?

[aaron.ballman]

clang-tidy/readability/ElseAfterReturnCheck.cpp

45 ↗

(On Diff #67347)

The deduced type for the code as-written is const char * const &, which isn't wrong, it's just not particularly useful. If you switch to const auto *, the deduced type is the much more reasonable const char *.

[omtcyfz]

clang-tidy/readability/ElseAfterReturnCheck.cpp

45 ↗

(On Diff #67347)

Ah... Thank you very much! Now I get it. I thought for whatever reason that there is implicit conversion to std::string.

[alexfh]

clang-tidy/readability/ElseAfterReturnCheck.cpp

45 ↗

(On Diff #67347)

This one *will* work in MSVC 2013

With MSVC you never know what will work, and what won't... Thanks for verifying!

[alexfh]

test/clang-tidy/readability-else-after-return.cpp

33 ↗

(On Diff #67347)

Why make comments and patterns unique?

Because FileCheck is way simpler than one usually thinks of it.

It doesn't maintain any relation between the location of the check line and the input line. So your assumption that

it will only look at the line above

is wrong.

The only thing FileCheck cares about is that the input contains a subsequence of lines that match all the CHECK patterns (a bit more complicated, if CHECK-NOT, CHECK-NEXT, CHECK-SAME, CHECK-DAG, or CHECK-LABEL are in use, but the basic idea is roughly the same).

So, if all CHECK-FIXES patterns in this test are // comment, then any input that contains at least the same number of // comment substrings on different lines will pass the test.

Please read http://llvm.org/docs/CommandGuide/FileCheck.html as well.

[omtcyfz]

Address comments.

[omtcyfz]

Make helper matcher const.

[aaron.ballman]

docs/clang-tidy/checks/readability-else-after-return.rst

8 ↗

(On Diff #67476)

enforcements (plural)

10 ↗

(On Diff #67476)

I would remove the "etc", since this check does not handle such cases.

12 ↗

(On Diff #67476)

Following -> The following
check would work -> the check works

14 ↗

(On Diff #67476)

Since we're talking about LLVM coding conventions, can you format this like LLVM code (elide braces, etc)?

14 ↗

(On Diff #67476)

This is not the proper way to introduce a code block. Should use .. code-block:: c++ and then indent the code blocks.

[alexfh]

clang-tidy/readability/ElseAfterReturnCheck.cpp

40 ↗

(On Diff #67476)

No braces around single-line if bodies, please. Clang-tidy code is mostly consistent wrt this.

docs/clang-tidy/checks/readability-else-after-return.rst

14 ↗

(On Diff #67476)

I think, the braces are fine, since the example needs to demonstrate that the check is able to remove them.

[aaron.ballman]

docs/clang-tidy/checks/readability-else-after-return.rst

14 ↗

(On Diff #67476)

Okay, that's a fair, but subtle, reason to leave them in.

[omtcyfz]

Address comments.

[aaron.ballman]

LGTM!

[omtcyfz]

Remove braces around single statement in for and if as Alex proposed.

[alexfh]

LG

clang-tools-extra/trunk/clang-tidy/readability/ElseAfterReturnCheck.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 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))))),
       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.
   // 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());
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/readability-else-after-return.rst

 .. title:: clang-tidy - readability-else-after-return
 
 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>`_.

clang-tools-extra/trunk/test/clang-tidy/readability-else-after-return.cpp

 // RUN: %check_clang_tidy %s readability-else-after-return %t
 
 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;
   }
 
   if (a > 0) {
     f(0);
     if (a > 10)
       return;
   } else {
     return;
   }
 
   if (a > 0)
     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++;
+    }
+  }
+}