[clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

[curdeius]

Summary

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 /* ... */;

[lebedev.ri]

I think it would be good to add some more explanation as to *why* that else has to be kept.

[curdeius]

In 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.

[curdeius]

Actually, I can make it an option for this check to skip or not constexpr ifs, WDYT?

[lebedev.ri]

In D53372#1268207, @curdeius wrote:

In 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 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.

[aaron.ballman]

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

10

Please add some of the warning text -- any warning will match this.

24

This seems unnecessary.

[curdeius]

Applied changes as per comments.

[aaron.ballman]

Hmm, the latest patch only seems to have the changes to the test but not the implementation?

[curdeius]

Fixed diff.

[aaron.ballman]

LGTM!

clang-tidy/readability/ElseAfterReturnCheck.cpp

-//===--- 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 &Result) {
-  const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
-  SourceLocation ElseLoc = If->getElseLoc();
-  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
+//===--- 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 &Result) {
+  const auto *If = Result.Nodes.getNodeAs<IfStmt>("if");
+  SourceLocation ElseLoc = If->getElseLoc();
+  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

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

+// 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;
+}