[clang-tools-extra] r278257 - [clang-tidy] enhance readability-else-after-return
Yung, Douglas via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 10 15:57:17 PDT 2016
Hi Kirill,
I believe your change is causing the PS4 bots to be red. Specifically, the test you added makes use of "throw" which doesn't work on the PS4 target since exceptions are disabled by default on the PS4 target. I think explicitly enabling exceptions should fix the test and propose the following patch:
Index: test/clang-tidy/readability-else-after-return.cpp
===================================================================
--- test/clang-tidy/readability-else-after-return.cpp (revision 278294)
+++ test/clang-tidy/readability-else-after-return.cpp (working copy)
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-else-after-return %t
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++11 -fexceptions
void f(int a) {
if (a > 0)
Thoughts?
Douglas Yung
> -----Original Message-----
> From: cfe-commits [mailto:cfe-commits-bounces at lists.llvm.org] On Behalf
> Of Kirill Bobyrev via cfe-commits
> Sent: Wednesday, August 10, 2016 11:06
> To: cfe-commits at lists.llvm.org
> Subject: [clang-tools-extra] r278257 - [clang-tidy] enhance
> readability-else-after-return
>
> 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=27825
> 7&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-
> afte
> +++ r-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-
> retur
> +++ n.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++;
> + }
> + }
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list