[clang-tidy] Add readability-use-early-exits check

[njames93]

Summary

Add a check to flag loops and functions where an if at the end of the body could be negated and made into an early exit.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst

115

Please add newline.

[njames93]

Add option to wrap early exit in braces.

[njames93]

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

218

I have an idea that we could infer this using the readability-braces-around-statements and its hicpp alias.
If enabled, probe the ShortStatementLines option to infer whether or not that check would fire if we make this transformation without using braces.

[njames93]

Added logic to infer WrapInBraces option if unspecified.

[njames93]

Fix pre-build failing.

[njames93]

Add option SplitConjunctions to alter fix-it for if statements with && in the condition.

[njames93]

NL

[njames93]

Fix typo in check list documentation.

[njames93]

Rebase and Ping??

[njames93]

Refactor some of the impl.

[njames93]

Ping?

[njames93]

Add NestedControlFlow options

This option can be used to silence diagnostics when there aren't any nested blocks inside the if statement.

[JonasToth]

just my 2 cents

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

66

did you consider comma expressions?

if (myDirtyCode(), myCondition && yourCondition). It seems to me, that the transformation would be incorrect.

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst

55

typo cunjunctions -> conjunctions

63

if this option is false, the transformation would be if(!(A && B)), right?

should demorgan rules be applied or at least be mentioned here? I think transforming to if (!A || !B) is at least a viable option for enough users.

112

maybe highlight the if as code with double quotes?

[njames93]

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

66

Comma operator is a binary operator, so the transformation would wrap the whole expression in parens.

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst

63

Once this is in, I plan to merge some common code with the simplify-boolean-expr logic for things like demorgan processing. Right now the transformation happens, the simplify boolean suggests a demorgan transformation of you run the output through clang tidy.

[JonasToth]

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst

63

a short reference to the readability-simplify-boolean-expr check in the user facing docs would be great.
i personally find it ok if the users "pipe" multiple checks together to reach a final transformation.

would this check then use the same settings as the readability-simplify-boolean-expr check? (that of course off topic and does not relate to this patch :) )

[njames93]

Fix documentation.

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst

63

I'm not sure it's really necessary to mention that the fix would likely need another fix, besides that comment would just be removed in the follow up.

[JonasToth]

_Edit_: If overloaded operators are ignored and the check only applies to pointers, int and bool the following is irrelevant. But I did not spot such a guard.
If this is the case, it of course opens the question about at least optional<> types and how to handle those.

IMHO the check looks good, but I do have concerns about correctness of the transformation in specific cases, especially overloaded operators.

If the conditions are inverted, it might be the case that the inverted operator is not overloaded, resulting in compilation errors.
I think that should be guarded against.

And then there is the more subtle issue of different semantics of the operators.

Hypothetical Matrix Algebra Situation:

// A, B are matrices of booleans with identical dimensions
// A && B will do '&&' on each matrix element
// The matrices overload 'operator bool()' for implicit conversion to 'bool',
// true := any element != false
// false := all false
if (A && B) {
  // !C inverts each boolean in C, same '&&' application
  if (B && !C) {
    padLines();
  }
}

Transformed to

if (!A )
  continue;
if ( !B)
  continue;
if (!B )
  continue;
if ( C)
  continue;
padLines();

is highly likely not a correct and equivalent transformation, both in this instance and in general for sure.

C++ allows to implement operators with different semantics and classes must not behave like 'int' for the operators. Such overloads are not necessarily incorrect. E.g. boost::sml uses the operator overloading to define state machines, which does not follow anything close to the semantics we need for this check.
Expression-template libraries (especially linear algebra and so on) might either break or change meaning as well.

In my personal opinion the check should at least have an option to disable transformations for classes with overloaded operators and verify that the transformation would still compile if done anyway.
I think even better would be a "concept check" to at least verify that the type in question models a boolean with its operators. But I am not sure how far such a check should go and I am aware that it would not be perfect anyway.

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

330

i would really prefer to just use strings here.
std::string Buff = CheckAlias.str() + OptName.str(); is much easier to understand and equivalent (?)

this function is called only once in the constructor as well, so speed and allocations are not valid in my opinion.

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst

63

ok, thats good with me.

[njames93]

In D130181#3769083, @JonasToth wrote:

...

Your concerns aren't actually that important. Because the transformations only work on for binary operators, and not CXXOperatorCallExpr, it would always never do any special logic, instead just wrap the whole thing in parens and negate it.

if (!(A && B))
  continue;

if (!(!B && C))
  continue;

padLines();

The only potential issue would be cases when the binary operator is type dependent, as binary operators where they type is unresolved are handled as BinaryOperators, even if every instantiation would be resolved to an operator call

template <size_t N, size_t M>
void fancyMatrix(Matrix<N,M> A, Matrix<M, M> B) {
  if (a && B) {
    ...
  }
}

I think the best way to resolve that could be delayed diagnostics for template operators.
So cache the locations of all dependent binary (and unary) operators, and see if any of them either stay as binary operators or get transformed into CXXOperatorCallExprs, if so don't emit the fix.
The second option is to just not emit any fix for template dependent operators.
WDYT?

[JonasToth]

In D130181#3769618, @njames93 wrote:

In D130181#3769083, @JonasToth wrote:

...

Your concerns aren't actually that important. Because the transformations only work on for binary operators, and not CXXOperatorCallExpr, it would always never do any special logic, instead just wrap the whole thing in parens and negate it.

Ah yes, you are right! That makes it very much better.

I think the best way to resolve that could be delayed diagnostics for template operators.
So cache the locations of all dependent binary (and unary) operators, and see if any of them either stay as binary operators or get transformed into CXXOperatorCallExprs, if so don't emit the fix.
The second option is to just not emit any fix for template dependent operators.
WDYT?

I am not 100% sure that delayed diagnostics would solve the issue _always_

template <typename T1, typename T2>
bool type_dependent_logic(T1 foo, T2 bar) {
    return foo && !bar;
}

This template would be a counter example to yours, where I feel less comfortable.
The definition itself has BinaryOperator in the AST.

Now different TUs might use this template wildly different, e.g. TU1.cpp only with builtins, making a transformation possible and TU2.cpp with fancy and weird types making the transformation not possible.

clang-tidy (e.g. with run-clang-tidy) would now still change the template definition after it received the diagnostic from TU1.cpp, even though in TU2.cpp it is not considered as valid.
This issue is common to many checks, e.g. the const-correctness suffered from it as well. In the const case I had to just had to consider type dependent expressions as modifications.

For TU-local templates your strategy is certainly viable. But in the general case I would rather not do it, or at least have a "default-off" option for it.

Finally, what are your thoughts on optional types? They are kinda bool in the sense of this check, but suffer from the shortcomings with overloaded operators.
Should they be explicitly not considered now (i am fine with that!)? In that case, it should be mentioned as limitation in the documentation.

[LegalizeAdulthood]

I'm not certain that the result of the transformation is more "readable"; is this check intended to aid conformance to a style guide?

[njames93]

In D130181#4213779, @LegalizeAdulthood wrote:

I'm not certain that the result of the transformation is more "readable"; is this check intended to aid conformance to a style guide?

One of the driving factors of this is that it can be used to remove nesting levels, which can definitely aid with readability.

[PiotrZSL]

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

360

This will trigger on all system code, and then users will complain again that they see poor clang-tidy performance...

when it could be just something like

ifStmt(unless(isExpansionInSystemHeader()),
       unless(isConsteval()),
       unless(isConstexpr()),
       unless(hasElse(stmt())),
       unless(hasInitStatement(stmt()),
       hasThen(compoundStmt(hasSizeAboeLineTreshold())),
       ...

Simply everything that could be put into matcher should be put into matcher (easier to maintain), also what's a point of checking functions that doesn't have if's. On that point also some implicit functions or if statement should be ignored, to avoid checking templates twice.

clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp

145

• Missing test with if in template function & implicit specialization, just to make sure that fixes won't get to mess up.
• Missing test with if under macro.
• Missing test with if under switch under loop (to check if brak/continue will be used correctly, instead of return).

[njames93]

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

360

Not triggering on system headers is a good point, but for raw performance, that code is better residing in the visitor, which can massively outperform matchers as we can straight up ignore huge blocks of code that aren't of interest.

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

add_clang_library(clangTidyReadabilityModule
   SimplifySubscriptExprCheck.cpp
   StaticAccessedThroughInstanceCheck.cpp
   StaticDefinitionInAnonymousNamespaceCheck.cpp
   StringCompareCheck.cpp
   SuspiciousCallArgumentCheck.cpp
   UniqueptrDeleteReleaseCheck.cpp
   UppercaseLiteralSuffixCheck.cpp
   UseAnyOfAllOfCheck.cpp
+  UseEarlyExitsCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyUtils
 
   DEPENDS
   omp_gen
   )

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "SimplifySubscriptExprCheck.h"
 #include "StaticAccessedThroughInstanceCheck.h"
 #include "StaticDefinitionInAnonymousNamespaceCheck.h"
 #include "StringCompareCheck.h"
 #include "SuspiciousCallArgumentCheck.h"
 #include "UniqueptrDeleteReleaseCheck.h"
 #include "UppercaseLiteralSuffixCheck.h"
 #include "UseAnyOfAllOfCheck.h"
+#include "UseEarlyExitsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SuspiciousCallArgumentCheck>(
         "readability-suspicious-call-argument");
     CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(
         "readability-uniqueptr-delete-release");
     CheckFactories.registerCheck<UppercaseLiteralSuffixCheck>(
         "readability-uppercase-literal-suffix");
     CheckFactories.registerCheck<UseAnyOfAllOfCheck>(
         "readability-use-anyofallof");
+    CheckFactories.registerCheck<UseEarlyExitsCheck>(
+        "readability-use-early-exits");
   }
 };
 
 // Register the ReadabilityModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ReadabilityModule>
     X("readability-module", "Adds readability-related checks.");
 
 } // namespace readability
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the ReadabilityModule.
 volatile int ReadabilityModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.h

+//===--- UseEarlyExitsCheck.h - clang-tidy ----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEEARLYEXITSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEEARLYEXITSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Finds `if` statements inside functions and loops that could be flipped to
+/// make an early exit.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/use-early-exits.html
+class UseEarlyExitsCheck : public ClangTidyCheck {
+public:
+  UseEarlyExitsCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+  unsigned getLineCountThreshold() const { return LineCountThreshold; }
+  bool getSplitConjunctions() const { return SplitConjunctions; }
+  bool getWrapInBraces() const { return WrapInBraces; }
+  bool getNestedControlFlow() const { return NestedControlFlow; }
+
+private:
+  const unsigned LineCountThreshold;
+  const bool SplitConjunctions;
+  const bool WrapInBraces;
+  const bool NestedControlFlow;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEEARLYEXITSCHECK_H

clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp

+//===--- UseEarlyExitsCheck.cpp - clang-tidy ------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseEarlyExitsCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/StmtCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+static bool needsParensAfterUnaryNegation(const Expr *E) {
+  E = E->IgnoreImpCasts();
+  if (isa<BinaryOperator>(E) || isa<ConditionalOperator>(E))
+    return true;
+
+  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(E))
+    return Op->getNumArgs() == 2 && Op->getOperator() != OO_Call &&
+           Op->getOperator() != OO_Subscript;
+
+  return false;
+}
+
+static void addReverseConditionFix(DiagnosticBuilder &Diag,
+                                   const Expr *Condition,
+                                   const ASTContext &Ctx) {
+  if (const auto *BO = dyn_cast<BinaryOperator>(Condition)) {
+    if (BO->isComparisonOp() && BO->getOpcode() != BO_Cmp) {
+      Diag << FixItHint::CreateReplacement(
+          BO->getOperatorLoc(),
+          BinaryOperator::getOpcodeStr(
+              BinaryOperator::negateComparisonOp(BO->getOpcode())));
+      return;
+    }
+  } else if (const auto *UO = dyn_cast<UnaryOperator>(Condition)) {
+    if (UO->getOpcode() == UO_LNot) {
+      Diag << FixItHint::CreateRemoval(UO->getOperatorLoc());
+      if (const auto *Parens = dyn_cast<ParenExpr>(UO->getSubExpr())) {
+        Diag << FixItHint::CreateRemoval(Parens->getLParen());
+        Diag << FixItHint::CreateRemoval(Parens->getRParen());
+      }
+      return;
+    }
+  } else if (const auto *BL = dyn_cast<CXXBoolLiteralExpr>(Condition)) {
+    Diag << FixItHint::CreateReplacement(BL->getSourceRange(),
+                                         BL->getValue() ? "false" : "true");
+    return;
+  } else if (const auto *IL = dyn_cast<IntegerLiteral>(Condition)) {
+    Diag << FixItHint::CreateReplacement(
+        IL->getSourceRange(), IL->getValue().getBoolValue() ? "0" : "1");
+    return;
+  }
+  if (needsParensAfterUnaryNegation(Condition)) {
+    Diag << FixItHint::CreateInsertion(Condition->getBeginLoc(), "!(")
+         << FixItHint::CreateInsertion(
+                Lexer::getLocForEndOfToken(Condition->getEndLoc(), 0,
+                                           Ctx.getSourceManager(),
+                                           Ctx.getLangOpts()),
+                ")");
+  } else {
+    Diag << FixItHint::CreateInsertion(Condition->getBeginLoc(), "!");
+  }
+}
+
+static void addConjunctionReverseOneStep(DiagnosticBuilder &Diag, const Expr *E,
+                                         const ASTContext &Ctx,
+                                         StringRef Replacement) {
+  const auto *BO = dyn_cast<BinaryOperator>(E);
+  if (BO && BO->getOpcode() == BO_LAnd) {
+    addConjunctionReverseOneStep(Diag, BO->getLHS(), Ctx, Replacement);
+    Diag << FixItHint::CreateReplacement(BO->getOperatorLoc(), Replacement);
+    addConjunctionReverseOneStep(Diag, BO->getRHS(), Ctx, Replacement);
+  } else {
+    addReverseConditionFix(Diag, E, Ctx);
+  }
+}
+
+static void addConjunctionReverseFix(DiagnosticBuilder &Diag,
+                                     const Expr *Condition,
+                                     const ASTContext &Ctx,
+                                     StringRef Continuation) {
+  const auto *BO = dyn_cast<BinaryOperator>(Condition);
+  if (!BO || BO->getOpcode() != BO_LAnd) {
+    addReverseConditionFix(Diag, Condition, Ctx);
+    return;
+  }
+  addConjunctionReverseOneStep(Diag, Condition, Ctx,
+                               SmallString<32>({") ", Continuation, "\nif ("}));
+}
+
+static llvm::iterator_range<CompoundStmt::const_reverse_body_iterator>
+getBodyReverse(const CompoundStmt *S) {
+  return {S->body_rbegin(), S->body_rend()};
+}
+
+static bool stmtHasNestedControlFlow(const Stmt *S) {
+  class NestedVisitor : public RecursiveASTVisitor<NestedVisitor> {
+    bool HasNested;
+
+  public:
+    bool TraverseStmt(Stmt *S, DataRecursionQueue *Queue = nullptr) {
+      if (isa<IfStmt, WhileStmt, DoStmt, SwitchStmt, ForStmt, CXXForRangeStmt,
+              CXXTryStmt, SEHTryStmt>(S)) {
+        HasNested = true;
+        return true;
+      }
+      return RecursiveASTVisitor<NestedVisitor>::TraverseStmt(S, Queue);
+    }
+    bool hasNested(const Stmt *S) {
+      HasNested = false;
+      TraverseStmt(const_cast<Stmt *>(S));
+      return HasNested;
+    }
+  };
+  return NestedVisitor{}.hasNested(S);
+}
+
+namespace {
+
+class EarlyExitVisitor : public RecursiveASTVisitor<EarlyExitVisitor> {
+  UseEarlyExitsCheck &Check;
+  ASTContext &Ctx;
+
+  bool isCandidateIf(const IfStmt *If) {
+    // Ignore const(expr|eval) if statements.
+    if (If->isConsteval() || If->isConstexpr())
+      return false;
+    // We can't transform if there is an else.
+    if (If->hasElseStorage())
+      return false;
+    // If there is a variable in the condition, This transformation would mean
+    // it goes out of scope before the current then branch can use it.
+    if (If->hasVarStorage())
+      return false;
+    // As above, only technically if the init doesn't actually have any decls,
+    // we could still do the transformation. But thats not exactly a common
+    // idiom.
+    if (If->hasInitStorage())
+      return false;
+    const auto *BodyCS = llvm::dyn_cast_or_null<CompoundStmt>(If->getThen());
+    if (!BodyCS)
+      return false;
+    if (Check.getNestedControlFlow() && !stmtHasNestedControlFlow(BodyCS))
+      return false;
+    const SourceManager &SM = Ctx.getSourceManager();
+    SourceLocation Begin = BodyCS->body_empty()
+                               ? BodyCS->getLBracLoc()
+                               : BodyCS->body_front()->getBeginLoc();
+    SourceLocation End = If->getEndLoc();
+    FileID BeginFile = SM.getFileID(Begin);
+    FileID EndFile = SM.getFileID(End);
+    if (BeginFile != EndFile)
+      return false;
+    if (Check.getLineCountThreshold()) {
+      unsigned BeginLine = SM.getSpellingLineNumber(Begin);
+      unsigned EndLine = SM.getSpellingLineNumber(End);
+      if (BeginLine > EndLine)
+        return false; // This case can't be good.
+      return (EndLine - BeginLine) >= Check.getLineCountThreshold();
+    }
+  }
+
+  void emitDiag(const IfStmt *If, StringRef Continuation,
+                ArrayRef<const Stmt *> RemoveItems = llvm::None) {
+    const auto *BodyCS = cast<CompoundStmt>(If->getThen());
+    auto Diag = Check.diag(If->getBeginLoc(), "use early exit")
+                << FixItHint::CreateInsertion(If->getThen()->getBeginLoc(),
+                                              Continuation)
+                << FixItHint::CreateRemoval(BodyCS->getLBracLoc())
+                << FixItHint::CreateRemoval(BodyCS->getRBracLoc());
+    if (Check.getSplitConjunctions())
+      addConjunctionReverseFix(Diag, If->getCond(), Ctx, Continuation);
+    else
+      addReverseConditionFix(Diag, If->getCond(), Ctx);
+    for (const Stmt *Item : RemoveItems) {
+      auto EndLoc = utils::lexer::getUnifiedEndLoc(
+          *Item, Ctx.getSourceManager(), Ctx.getLangOpts());
+      if (!EndLoc.isValid())
+        EndLoc = Item->getEndLoc();
+      auto BeginLoc = Item->getBeginLoc();
+      if (BeginLoc.isFileID() && EndLoc.isFileID() &&
+          Ctx.getSourceManager().isWrittenInSameFile(BeginLoc, EndLoc)) {
+        Diag << FixItHint::CreateRemoval({BeginLoc, EndLoc});
+      } else {
+        // Stop if we fail to remove an item.
+        break;
+      }
+    }
+  }
+
+public:
+  EarlyExitVisitor(UseEarlyExitsCheck &Check, ASTContext &Ctx)
+      : Check(Check), Ctx(Ctx) {}
+
+  bool traverse() { return TraverseAST(Ctx); }
+
+  bool VisitFunctionDecl(const FunctionDecl *Func) {
+    // Skip any declarations.
+    if (!Func->doesThisDeclarationHaveABody())
+      return true;
+    // Just ignore no return functions.
+    if (Func->isNoReturn())
+      return true;
+    // Early exit logic only works for functions that return void.
+    // FIXME: Explore options where following the IfStmt there is a return
+    // value with a literal return.
+    // bool hasSomething(Class *S) {
+    //   if (S) {
+    //     return S->hasSomething();
+    //   }
+    //   return false;
+    // }
+    // bool hasSomething(Class *S) {
+    //   if (!S)
+    //     return false;
+    //   return S->hasSomething();
+    //   return false; // Ideally this would be removed too.
+    // }
+    if (Func->getReturnType()->isVoidType())
+      checkBodyReturnVoid(cast<CompoundStmt>(Func->getBody()));
+    return true;
+  }
+
+  void checkBodyReturnVoid(const CompoundStmt *CS) {
+    SmallVector<const Stmt *, 2> RemoveItems;
+    for (const Stmt *Item : getBodyReverse(CS)) {
+      // If the last statement in the function is an empty, ignore it.
+      if (isa<ReturnStmt>(Item) &&
+          cast<ReturnStmt>(Item)->getRetValue() == nullptr) {
+        RemoveItems.push_back(Item);
+        continue;
+      }
+      // While were here, we can safely ignore empty null stmts.
+      if (isa<NullStmt>(Item) &&
+          !cast<NullStmt>(Item)->hasLeadingEmptyMacro()) {
+        RemoveItems.push_back(Item);
+        continue;
+      }
+      if (const auto *If = dyn_cast<IfStmt>(Item)) {
+        if (isCandidateIf(If)) {
+          emitDiag(If, Check.getWrapInBraces() ? "{\nreturn;\n}" : "\nreturn;",
+                   RemoveItems);
+          checkBodyReturnVoid(cast<CompoundStmt>(If->getThen()));
+        }
+      }
+      break;
+    }
+  }
+
+  void checkBodyContinue(const CompoundStmt *CS) {
+    SmallVector<const Stmt *, 2> RemoveItems;
+    for (const Stmt *Item : getBodyReverse(CS)) {
+      // If the last statement in the block is a continue, ignore it.
+      if (isa<ContinueStmt>(Item)) {
+        RemoveItems.push_back(Item);
+
+        continue;
+      }
+      // While were here, we can safely ignore empty null stmts.
+      if (isa<NullStmt>(Item) &&
+          !cast<NullStmt>(Item)->hasLeadingEmptyMacro()) {
+        RemoveItems.push_back(Item);
+        continue;
+      }
+      if (const auto *If = dyn_cast<IfStmt>(Item)) {
+        if (isCandidateIf(If)) {
+          emitDiag(If,
+                   Check.getWrapInBraces() ? "{\ncontinue;\n}" : "\ncontinue;",
+                   RemoveItems);
+          checkBodyContinue(cast<CompoundStmt>(If->getThen()));
+        }
+      }
+      break;
+    }
+  }
+
+  bool VisitForStmt(const ForStmt *For) {
+    if (const auto *CS = dyn_cast_or_null<CompoundStmt>(For->getBody()))
+      checkBodyContinue(CS);
+    return true;
+  }
+
+  bool VisitWhileStmt(const WhileStmt *While) {
+    if (const auto *CS = dyn_cast_or_null<CompoundStmt>(While->getBody()))
+      checkBodyContinue(CS);
+    return true;
+  }
+
+  bool VisitDoStmt(const DoStmt *Do) {
+    if (const auto *CS = dyn_cast_or_null<CompoundStmt>(Do->getBody()))
+      checkBodyContinue(CS);
+    return true;
+  }
+
+  bool VisitCXXForRangeStmt(const CXXForRangeStmt *ForRange) {
+    if (const auto *CS = dyn_cast_or_null<CompoundStmt>(ForRange->getBody()))
+      checkBodyContinue(CS);
+    return true;
+  }
+};
+} // namespace
+
+static bool fallbackBracesCheckActive(llvm::Optional<bool> Cur,
+                                      ClangTidyContext *Context) {
+  if (Cur)
+    return *Cur;
+  static constexpr size_t BSize = 64; // Should only ever need 63 chars here.
+  char Buff[BSize];
+  constexpr llvm::StringLiteral OptName = ".ShortStatementLines";
+  constexpr llvm::StringLiteral CheckAliases[] = {
+      "google-readability-braces-around-statements",
+      "readability-braces-around-statements", "hicpp-braces-around-statements"};
+  for (StringRef CheckAlias : CheckAliases) {
+    if (!Context->isCheckEnabled(CheckAlias))
+      continue;
+    assert(BSize >= CheckAlias.size() + OptName.size());
+    memcpy(Buff, CheckAlias.data(), CheckAlias.size());
+    memcpy(Buff + CheckAlias.size(), OptName.data(), OptName.size());
+    auto Iter = Context->getOptions().CheckOptions.find(
+        {Buff, CheckAlias.size() + OptName.size()});
+    if (Iter == Context->getOptions().CheckOptions.end())
+      return true;
+    unsigned Value;
+    if (StringRef(Iter->getValue().Value).getAsInteger(10, Value) || Value < 2)
+      return true;
+  }
+  return false;
+}
+
+UseEarlyExitsCheck::UseEarlyExitsCheck(StringRef Name,
+                                       ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      LineCountThreshold(Options.get("LineCountThreshold", 10U)),
+      SplitConjunctions(Options.get("SplitConjunctions", false)),
+      WrapInBraces(fallbackBracesCheckActive(Options.get<bool>("WrapInBraces"),
+                                             Context)),
+      NestedControlFlow(Options.get("NestedControlFlow", false)) {}
+
+void UseEarlyExitsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "LineCountThreshold", LineCountThreshold);
+  Options.store(Opts, "SplitConjunctions", SplitConjunctions);
+  Options.store(Opts, "WrapInBraces", WrapInBraces);
+  Options.store(Opts, "NestedControlFlow", NestedControlFlow);
+}
+
+void UseEarlyExitsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(translationUnitDecl(), this);
+}
+
+void UseEarlyExitsCheck::check(const MatchFinder::MatchResult &Result) {
+  EarlyExitVisitor(*this, *Result.Context).traverse();
+}
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 New checks
 ^^^^^^^^^^
 
 - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
   <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check.
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
+- New :doc:`readability-use-early-exits
+  <clang-tidy/checks/readability/use-early-exits>` check.
+
+  Finds ``if`` statements inside functions and loops that could be flipped to 
+  make an early exit.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `readability-simplify-subscript-expr <readability/simplify-subscript-expr.html>`_, "Yes"
    `readability-static-accessed-through-instance <readability/static-accessed-through-instance.html>`_, "Yes"
    `readability-static-definition-in-anonymous-namespace <readability/static-definition-in-anonymous-namespace.html>`_, "Yes"
    `readability-string-compare <readability/string-compare.html>`_, "Yes"
    `readability-suspicious-call-argument <readability/suspicious-call-argument.html>`_,
    `readability-uniqueptr-delete-release <readability/uniqueptr-delete-release.html>`_, "Yes"
    `readability-uppercase-literal-suffix <readability/uppercase-literal-suffix.html>`_, "Yes"
    `readability-use-anyofallof <readability/use-anyofallof.html>`_,
+   `readability-use-early-exits <readability/use-early-exits.html>`_, "Yes"
    `zircon-temporary-objects <zircon/temporary-objects.html>`_,
 
 
 .. csv-table:: Aliases..
    :header: "Name", "Redirect", "Offers fixes"
 
    `bugprone-narrowing-conversions <bugprone/narrowing-conversions.html>`_, `cppcoreguidelines-narrowing-conversions <cppcoreguidelines/narrowing-conversions.html>`_,
    `cert-con36-c <cert/con36-c.html>`_, `bugprone-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions.html>`_,

clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst

+.. title:: clang-tidy - readability-use-early-exits
+
+readability-use-early-exits
+===========================
+
+Finds ``if`` statements inside functions and loops that could be flipped to 
+make an early exit.
+
+Example:
+
+.. code-block:: c++
+
+  void Process(MyClass* C) {
+    if (C) {
+      // Do some long processing.
+    }
+  }
+
+  void Process(span<MyClass*> C){
+    for (MyClass *Item : C) {
+      if (Item) {
+        // Do some long processing.
+      }
+    }
+  }
+
+Would be transformed to:
+
+.. code-block:: c++
+
+  void Process(MyClass* C) {
+    if (!C)
+      return;
+    // Do some long processing.
+  }
+
+  void Process(span<MyClass*> C){
+    for (MyClass *Item : C) {
+      if (!Item)
+        continue;
+      // Do some long processing.
+    }
+  }
+
+Options
+-------
+
+.. option:: LineCountThreshold
+
+  Don't transform any ``if`` statement if the statement uses less than this 
+  many lines. Default value is `10`.
+
+.. option:: SplitConjunctions
+
+  If `true`, split up conditions with cunjunctions (``&&``) into multiple 
+  ``if`` statements. Default value is `false`.
+
+  When `true`:
+
+  .. code-block:: c++
+
+    void Process(bool A, bool B) {
+      if (A && B) {
+        // Long processing.
+      }
+    }
+
+  Would be transformed to:
+
+  .. code-block:: c++
+
+    void Process(bool A, bool B) {
+      if (!A)
+        return;
+
+      if (!B)
+        return;
+        
+      // Long processing.
+    }
+
+.. option:: WrapInBraces
+
+  If `true`, the early exit will be wrapped in braces.
+
+  If unspecified, the value will be inferred based on whether
+  :doc:`readability-braces-around-statements <braces-around-statements>` or one
+  of its alias' are active.
+
+  When `true`, The above exmples would be transformed to:
+
+  .. code-block:: c++
+
+    void Process(MyClass *C) {
+      if (!C) {
+        return;
+      }
+      // Do some long processing
+    }
+
+    void Process(span<MyClass*> C){
+      for (MyClass *Item : C) {
+        if (!Item) {
+          continue;
+        }
+        // Do some long processing.
+      }
+    }
+
+.. option:: NestedControlFlow
+
+  If `true`, the check will only report ifs which contain nested control blocks.
+  Default value is `false`
+

clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-braces.cpp

+// RUN: %check_clang_tidy %s readability-use-early-exits,google-readability-braces-around-statements %t -- \
+// RUN:   -config="{CheckOptions: {readability-use-early-exits.LineCountThreshold: 1, \
+// RUN:                            google-readability-braces-around-statements.ShortStatementLines: 0}}"
+
+// RUN: %check_clang_tidy %s readability-use-early-exits,hicpp-braces-around-statements -format-style=llvm %t -- \
+// RUN:   -config="{CheckOptions: {readability-use-early-exits.LineCountThreshold: 1}}"
+void nomralFunc(int *A) {
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: use early exit
+  if (A) {
+    *A = 10;
+  }
+  //       CHECK-FIXES: if (!A) {
+  //  CHECK-FIXES-NEXT: return;
+  //  CHECK-FIXES-NEXT: }
+  //  CHECK-FIXES-NEXT: *A = 10;
+}

clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-nested.cpp

+// RUN: %check_clang_tidy %s readability-use-early-exits %t -- \
+// RUN:   -config="{CheckOptions: {readability-use-early-exits.LineCountThreshold: 1, \
+// RUN:                            readability-use-early-exits.NestedControlFlow: true}}"
+
+void padLines();
+
+void foo(bool A, bool B) {
+  while (true) {
+    if (A) {
+      padLines();
+    }
+  }
+
+  while (true) {
+    // CHECK-MESSAGES: [[@LINE+1]]:5: warning: use early exit
+    if (A) {
+      if (B) {
+        padLines();
+      }
+    }
+  }
+
+  while (true) {
+    // CHECK-MESSAGES: [[@LINE+1]]:5: warning: use early exit
+    if (A) {
+      while (true) {
+        padLines();
+      }
+    }
+  }
+
+  while (true) {
+    // CHECK-MESSAGES: [[@LINE+1]]:5: warning: use early exit
+    if (A) {
+      switch (B) {
+      case true:
+      case false:
+        break;
+      }
+    }
+  }
+}

clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-split.cpp

+// RUN: %check_clang_tidy %s readability-use-early-exits %t -- \
+// RUN:   -config="{CheckOptions: {readability-use-early-exits.LineCountThreshold: 1, \
+// RUN:                            readability-use-early-exits.SplitConjunctions: true}}"
+
+void padLines();
+
+bool A, B, C;
+
+void conjunction() {
+  // CHECK-MESSAGES: [[@LINE+2]]:5: warning: use early exit
+  while (true) {
+    if (A && B) {
+      padLines();
+    }
+  }
+
+  //       CHECK-FIXES: while (true) {
+  //  CHECK-FIXES-NEXT:   if (!A )
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   if ( !B)
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   padLines();
+
+  // CHECK-MESSAGES: [[@LINE+2]]:5: warning: use early exit
+  while (true) {
+    if (A && B && !C) {
+      padLines();
+    }
+  }
+
+  //       CHECK-FIXES: while (true) {
+  //  CHECK-FIXES-NEXT:   if (!A )
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   if ( !B )
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   if ( C)
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   padLines();
+
+  // CHECK-MESSAGES: [[@LINE+3]]:5: warning: use early exit
+  // CHECK-MESSAGES: [[@LINE+3]]:7: warning: use early exit
+  while (true) {
+    if (A && B) {
+      if (B && !C) {
+        padLines();
+      }
+    }
+  }
+
+  //       CHECK-FIXES: while (true) {
+  //  CHECK-FIXES-NEXT:   if (!A )
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   if ( !B)
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   if (!B )
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   if ( C)
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   padLines();
+}
+
+void disjunction() {
+  // CHECK-MESSAGES: [[@LINE+2]]:5: warning: use early exit
+  while (true) {
+    if (A || B) {
+      padLines();
+    }
+  }
+
+  //       CHECK-FIXES: while (true) {
+  //  CHECK-FIXES-NEXT:   if (!(A || B))
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   padLines();
+}

clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp

+// RUN: %check_clang_tidy %s readability-use-early-exits %t -- \
+// RUN:   -config="{CheckOptions: {readability-use-early-exits.LineCountThreshold: 1}}"
+
+// Run the check with the braces around statements check also enabled, but
+// ShortStatementLines is set so that we shouldn't add braces.
+// RUN: %check_clang_tidy %s readability-use-early-exits,readability-braces-around-statements -format-style=llvm %t -- \
+// RUN:   -config="{CheckOptions: {readability-use-early-exits.LineCountThreshold: 1, \
+// RUN:                            readability-braces-around-statements.ShortStatementLines: 2}}"
+
+// Just to hit the threshold
+void padLines();
+
+void nomralFunc(int *A) {
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: use early exit
+  if (A) {
+    *A = 10;
+  }
+  //       CHECK-FIXES: if (!A)
+  //  CHECK-FIXES-NEXT: return;
+  //  CHECK-FIXES-NEXT: *A = 10;
+}
+
+void funcWithTrailing(int *B) {
+  // CHECK-MESSAGES: [[@LINE+1]]:3: warning: use early exit
+  if (B) {
+    *B = 10;
+  }
+  return;
+  ;
+  //       CHECK-FIXES: if (!B)
+  //  CHECK-FIXES-NEXT: return;
+  //  CHECK-FIXES-NEXT: *B = 10;
+}
+
+void normal(int A, int B) {
+
+  while (true) { // CHECK-MESSAGES: [[@LINE+1]]:5: warning: use early exit
+    if (A == B) {
+      padLines();
+    }
+  }
+  //       CHECK-FIXES:  while (true) {
+  //  CHECK-FIXES-NEXT: if (A != B)
+  //  CHECK-FIXES-NEXT:   continue;
+  //  CHECK-FIXES-NEXT: padLines();
+
+  // Eat continue and empty nulls
+  while (true) { // CHECK-MESSAGES: [[@LINE+1]]:5: warning: use early exit
+    if (A != B) {
+      padLines();
+    }
+    continue;
+    ;
+    continue;
+  }
+  //       CHECK-FIXES:  while (true) {
+  //  CHECK-FIXES-NEXT: if (A == B)
+  //  CHECK-FIXES-NEXT:   continue;
+  //  CHECK-FIXES-NEXT: padLines();
+}
+
+void toShort(int A, int B) {
+  while (true) {
+    if (A == B) 
+    { }
+  }
+}
+
+void hasElse(int A, int B) {
+  while (true) {
+    if (A == B) {
+
+    } else {
+    }
+  }
+}
+
+void hasTrailingStmt(int A, int B) {
+  while (true) {
+    if (A == B) {
+    }
+    padLines();
+  }
+}
+
+void nested(int A, int B) {
+  // if (A > B) {
+  // CHECK-MESSAGES: [[@LINE+6]]:5: warning: use early exit
+  // if (B < A) {
+  // CHECK-MESSAGES: [[@LINE+5]]:7: warning: use early exit
+  // if (A == B) {
+  // CHECK-MESSAGES: [[@LINE+4]]:9: warning: use early exit
+  while (true) { // Nested
+    if (A > B) {
+      if (B < A) {
+        if (A != B) {
+          padLines();
+        }
+      }
+    }
+  } // EndLoop
+  //       CHECK-FIXES: while (true) { // Nested
+  //  CHECK-FIXES-NEXT: if (A <= B)
+  //  CHECK-FIXES-NEXT:   continue;
+  //  CHECK-FIXES-NEXT: if (B >= A)
+  //  CHECK-FIXES-NEXT:   continue;
+  //  CHECK-FIXES-NEXT: if (A == B)
+  //  CHECK-FIXES-NEXT:   continue;
+  //  CHECK-FIXES-NEXT: padLines();
+}
+
+void badNested(bool B) {
+  // Ensure check doesn't check for nested `if` if outer `if` has else.
+  while (true) {
+    if (B) {
+      if (B) {
+      }
+    } else {
+    }
+  }
+}
+
+void semiNested(int A, int B) {
+  // CHECK-MESSAGES: [[@LINE+2]]:5: warning: use early exit
+  while (true) { // SemiNested
+    if (A > B) {
+      if (B < A) {
+        if (A != B) {
+          padLines();
+        }
+      } else {
+      }
+    }
+  }
+  //       CHECK-FIXES: while (true) { // SemiNested
+  //  CHECK-FIXES-NEXT:   if (A <= B)
+  //  CHECK-FIXES-NEXT:     continue;
+  //  CHECK-FIXES-NEXT:   if (B < A) {
+  //  CHECK-FIXES-NEXT:     if (A != B) {
+  //  CHECK-FIXES-NEXT:       padLines();
+  //  CHECK-FIXES-NEXT:     }
+  //  CHECK-FIXES-NEXT:     } else {
+  //  CHECK-FIXES-NEXT:     }
+}