diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -48,6 +48,7 @@ UniqueptrDeleteReleaseCheck.cpp UppercaseLiteralSuffixCheck.cpp UseAnyOfAllOfCheck.cpp + UseEarlyExitsCheck.cpp LINK_LIBS clangTidy diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -51,6 +51,7 @@ #include "UniqueptrDeleteReleaseCheck.h" #include "UppercaseLiteralSuffixCheck.h" #include "UseAnyOfAllOfCheck.h" +#include "UseEarlyExitsCheck.h" namespace clang { namespace tidy { @@ -143,6 +144,8 @@ "readability-uppercase-literal-suffix"); CheckFactories.registerCheck( "readability-use-anyofallof"); + CheckFactories.registerCheck( + "readability-use-early-exits"); } }; diff --git a/clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.h b/clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.h @@ -0,0 +1,46 @@ +//===--- 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 diff --git a/clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp @@ -0,0 +1,367 @@ +//===--- 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(E) || isa(E)) + return true; + + if (const auto *Op = dyn_cast(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(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(Condition)) { + if (UO->getOpcode() == UO_LNot) { + Diag << FixItHint::CreateRemoval(UO->getOperatorLoc()); + if (const auto *Parens = dyn_cast(UO->getSubExpr())) { + Diag << FixItHint::CreateRemoval(Parens->getLParen()); + Diag << FixItHint::CreateRemoval(Parens->getRParen()); + } + return; + } + } else if (const auto *BL = dyn_cast(Condition)) { + Diag << FixItHint::CreateReplacement(BL->getSourceRange(), + BL->getValue() ? "false" : "true"); + return; + } else if (const auto *IL = dyn_cast(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(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(Condition); + if (!BO || BO->getOpcode() != BO_LAnd) { + addReverseConditionFix(Diag, Condition, Ctx); + return; + } + addConjunctionReverseOneStep(Diag, Condition, Ctx, + SmallString<32>({") ", Continuation, "\nif ("})); +} + +static llvm::iterator_range +getBodyReverse(const CompoundStmt *S) { + return {S->body_rbegin(), S->body_rend()}; +} + +static bool stmtHasNestedControlFlow(const Stmt *S) { + class NestedVisitor : public RecursiveASTVisitor { + bool HasNested; + + public: + bool TraverseStmt(Stmt *S, DataRecursionQueue *Queue = nullptr) { + if (isa(S)) { + HasNested = true; + return true; + } + return RecursiveASTVisitor::TraverseStmt(S, Queue); + } + bool hasNested(const Stmt *S) { + HasNested = false; + TraverseStmt(const_cast(S)); + return HasNested; + } + }; + return NestedVisitor{}.hasNested(S); +} + +namespace { + +class EarlyExitVisitor : public RecursiveASTVisitor { + 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(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 RemoveItems = llvm::None) { + const auto *BodyCS = cast(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(Func->getBody())); + return true; + } + + void checkBodyReturnVoid(const CompoundStmt *CS) { + SmallVector RemoveItems; + for (const Stmt *Item : getBodyReverse(CS)) { + // If the last statement in the function is an empty, ignore it. + if (isa(Item) && + cast(Item)->getRetValue() == nullptr) { + RemoveItems.push_back(Item); + continue; + } + // While were here, we can safely ignore empty null stmts. + if (isa(Item) && + !cast(Item)->hasLeadingEmptyMacro()) { + RemoveItems.push_back(Item); + continue; + } + if (const auto *If = dyn_cast(Item)) { + if (isCandidateIf(If)) { + emitDiag(If, Check.getWrapInBraces() ? "{\nreturn;\n}" : "\nreturn;", + RemoveItems); + checkBodyReturnVoid(cast(If->getThen())); + } + } + break; + } + } + + void checkBodyContinue(const CompoundStmt *CS) { + SmallVector RemoveItems; + for (const Stmt *Item : getBodyReverse(CS)) { + // If the last statement in the block is a continue, ignore it. + if (isa(Item)) { + RemoveItems.push_back(Item); + + continue; + } + // While were here, we can safely ignore empty null stmts. + if (isa(Item) && + !cast(Item)->hasLeadingEmptyMacro()) { + RemoveItems.push_back(Item); + continue; + } + if (const auto *If = dyn_cast(Item)) { + if (isCandidateIf(If)) { + emitDiag(If, + Check.getWrapInBraces() ? "{\ncontinue;\n}" : "\ncontinue;", + RemoveItems); + checkBodyContinue(cast(If->getThen())); + } + } + break; + } + } + + bool VisitForStmt(const ForStmt *For) { + if (const auto *CS = dyn_cast_or_null(For->getBody())) + checkBodyContinue(CS); + return true; + } + + bool VisitWhileStmt(const WhileStmt *While) { + if (const auto *CS = dyn_cast_or_null(While->getBody())) + checkBodyContinue(CS); + return true; + } + + bool VisitDoStmt(const DoStmt *Do) { + if (const auto *CS = dyn_cast_or_null(Do->getBody())) + checkBodyContinue(CS); + return true; + } + + bool VisitCXXForRangeStmt(const CXXForRangeStmt *ForRange) { + if (const auto *CS = dyn_cast_or_null(ForRange->getBody())) + checkBodyContinue(CS); + return true; + } +}; +} // namespace + +static bool fallbackBracesCheckActive(llvm::Optional 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("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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,12 @@ Warns when a struct or class uses const or reference (lvalue or rvalue) data members. +- New :doc:`readability-use-early-exits + ` check. + + Finds ``if`` statements inside functions and loops that could be flipped to + make an early exit. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -362,6 +362,7 @@ `readability-uniqueptr-delete-release `_, "Yes" `readability-uppercase-literal-suffix `_, "Yes" `readability-use-anyofallof `_, + `readability-use-early-exits `_, "Yes" `zircon-temporary-objects `_, diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst @@ -0,0 +1,113 @@ +.. 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 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 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 conjunctions (``&&``) 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 ` 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 C){ + for (MyClass *Item : C) { + if (!Item) { + continue; + } + // Do some long processing. + } + } + +.. option:: NestedControlFlow + + If `true`, the check will only report ``if`` statements which contain nested + control blocks. Default value is `false`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-braces.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-braces.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-braces.cpp @@ -0,0 +1,16 @@ +// 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; +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-nested.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-nested.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-nested.cpp @@ -0,0 +1,42 @@ +// 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; + } + } + } +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-split.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-split.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits-split.cpp @@ -0,0 +1,74 @@ +// 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(); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp @@ -0,0 +1,144 @@ +// 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: } +}