njames93 created this revision.
njames93 added reviewers: alexfh, aaron.ballman.
Herald added subscribers: carlosgalvezp, xazax.hun, mgorny.
Herald added a project: All.
njames93 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D130181
Files:
clang-tools-extra/clang-tidy/readability/CMakeLists.txt
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability/use-early-exits.cpp
@@ -0,0 +1,158 @@
+// RUN: %check_clang_tidy %s readability-use-early-exits -format-style=llvm %t -- \
+// RUN: -config="{CheckOptions: {readability-use-early-exits.LineCountThreshold: 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-EMPTY:
+ // 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-EMPTY:
+ // CHECK-FIXES-NEXT: *B = 10;
+ // CHECK-FIXES-EMPTY:
+ // CHECK-FIXES-NEXT: return;
+ // CHECK-FIXES-NEXT: ;
+}
+
+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-EMPTY:
+ // CHECK-FIXES-NEXT: padLines();
+ // CHECK-FIXES-NEXT: }
+
+ // 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-EMPTY:
+ // CHECK-FIXES-NEXT: padLines();
+ // CHECK-FIXES-EMPTY:
+ // CHECK-FIXES-NEXT: continue;
+ // CHECK-FIXES-NEXT: ;
+ // CHECK-FIXES-NEXT: continue;
+ // CHECK-FIXES-NEXT: }
+}
+
+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-EMPTY:
+ // CHECK-FIXES-NEXT: if (B >= A)
+ // CHECK-FIXES-NEXT: continue;
+ // CHECK-FIXES-EMPTY:
+ // CHECK-FIXES-NEXT: if (A == B)
+ // CHECK-FIXES-NEXT: continue;
+ // CHECK-FIXES-EMPTY:
+ // CHECK-FIXES-NEXT: padLines();
+ // CHECK-FIXES-EMPTY:
+ // CHECK-FIXES-NEXT: } // EndLoop
+}
+
+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-EMPTY:
+ // 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: }
+ // CHECK-FIXES-NEXT: }
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst
@@ -0,0 +1,51 @@
+.. 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.
+ }
+ }
+ }
+
+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`.
\ No newline at end of file
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -359,6 +359,7 @@
`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>`_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -150,6 +150,12 @@
Future libc++ will remove the extension (`D120996
<https://reviews.llvm.org/D120996>`).
+- 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
^^^^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.h
@@ -0,0 +1,40 @@
+//===--- 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; }
+
+private:
+ const unsigned LineCountThreshold;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEEARLYEXITSCHECK_H
Index: clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp
@@ -0,0 +1,228 @@
+//===--- 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 "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchers.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 std::vector<FixItHint> createReverseConditionFix(const Expr *Condition,
+ const ASTContext &Ctx) {
+ if (const auto *BO = dyn_cast<BinaryOperator>(Condition)) {
+ switch (BO->getOpcode()) {
+ case BO_EQ:
+ return {FixItHint::CreateReplacement(BO->getOperatorLoc(), "!=")};
+ case BO_NE:
+ return {FixItHint::CreateReplacement(BO->getOperatorLoc(), "==")};
+ case BO_LT:
+ return {FixItHint::CreateReplacement(BO->getOperatorLoc(), ">=")};
+ case BO_LE:
+ return {FixItHint::CreateReplacement(BO->getOperatorLoc(), ">")};
+ case BO_GT:
+ return {FixItHint::CreateReplacement(BO->getOperatorLoc(), "<=")};
+ case BO_GE:
+ return {FixItHint::CreateReplacement(BO->getOperatorLoc(), "<")};
+ default:
+ break;
+ }
+ } else if (const auto *UO = dyn_cast<UnaryOperator>(Condition)) {
+ if (UO->getOpcode() == UO_LNot)
+ return {FixItHint::CreateRemoval(UO->getOperatorLoc())};
+ } else if (const auto *BL = dyn_cast<CXXBoolLiteralExpr>(Condition)) {
+ return {FixItHint::CreateReplacement(BL->getSourceRange(),
+ BL->getValue() ? "false" : "true")};
+ } else if (const auto *IL = dyn_cast<IntegerLiteral>(Condition)) {
+ return {FixItHint::CreateReplacement(
+ IL->getSourceRange(), IL->getValue().getBoolValue() ? "0" : "1")};
+ }
+ std::vector<FixItHint> FixIts;
+ if (needsParensAfterUnaryNegation(Condition)) {
+ FixIts.push_back(
+ FixItHint::CreateInsertion(Condition->getBeginLoc(), "!("));
+ FixIts.push_back(FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Condition->getEndLoc(), 0,
+ Ctx.getSourceManager(), Ctx.getLangOpts()),
+ ")"));
+ } else {
+ FixIts.push_back(FixItHint::CreateInsertion(Condition->getBeginLoc(), "!"));
+ }
+ return FixIts;
+}
+
+static llvm::iterator_range<CompoundStmt::const_reverse_body_iterator>
+getBodyReverse(const CompoundStmt *S) {
+ return {S->body_rbegin(), S->body_rend()};
+}
+
+namespace {
+
+class EarlyExitVisitor : public RecursiveASTVisitor<EarlyExitVisitor> {
+ UseEarlyExitsCheck &Check;
+ ASTContext &Ctx;
+
+public:
+ EarlyExitVisitor(UseEarlyExitsCheck &Check, ASTContext &Ctx)
+ : Check(Check), Ctx(Ctx) {}
+
+ bool traverse() { return TraverseAST(Ctx); }
+
+ template <typename TerminationStmt>
+ void diagnoseIf(const IfStmt *If, StringRef Continuation);
+
+ template <typename TerminationStmt>
+ void checkBody(const CompoundStmt *CS, StringRef Continuation) {
+ for (const Stmt *Item : getBodyReverse(CS)) {
+ // If the last statement in the function is a return, ignore it.
+ if (isa<TerminationStmt>(Item))
+ continue;
+ // While were here, we can safely ignore empty null stmts.
+ if (isa<NullStmt>(Item) && !cast<NullStmt>(Item)->hasLeadingEmptyMacro())
+ continue;
+ if (const auto *If = dyn_cast<IfStmt>(Item))
+ diagnoseIf<TerminationStmt>(If, Continuation);
+ break;
+ }
+ }
+
+ 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())
+ return true;
+ // FIXME: explore if CoreturnStmt could work here also.
+ checkBody<ReturnStmt>(cast<CompoundStmt>(Func->getBody()), "return");
+ return true;
+ }
+
+ bool VisitForStmt(const ForStmt *For) {
+ if (const auto *CS = dyn_cast_or_null<CompoundStmt>(For->getBody())) {
+ checkBody<ContinueStmt>(CS, "continue");
+ }
+ return true;
+ }
+
+ bool VisitWhileStmt(const WhileStmt *While) {
+ if (const auto *CS = dyn_cast_or_null<CompoundStmt>(While->getBody())) {
+ checkBody<ContinueStmt>(CS, "continue");
+ }
+ return true;
+ }
+
+ bool VisitDoStmt(const DoStmt *Do) {
+ if (const auto *CS = dyn_cast_or_null<CompoundStmt>(Do->getBody())) {
+ checkBody<ContinueStmt>(CS, "continue");
+ }
+ return true;
+ }
+
+ bool VisitCXXForRangeStmt(const CXXForRangeStmt *ForRange) {
+ if (const auto *CS = dyn_cast_or_null<CompoundStmt>(ForRange->getBody())) {
+ checkBody<ContinueStmt>(CS, "continue");
+ }
+ return true;
+ }
+};
+
+template <typename TerminationStmt>
+void EarlyExitVisitor::diagnoseIf(const IfStmt *If, StringRef Continuation) {
+ // Ignore const(expr|eval) if statements.
+ if (If->isConsteval() || If->isConstexpr())
+ return;
+ // We can't transform if there is an else.
+ if (If->hasElseStorage())
+ return;
+ // 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;
+ // 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;
+ const auto *BodyCS = llvm::dyn_cast_or_null<CompoundStmt>(If->getThen());
+ if (!BodyCS)
+ return;
+ const SourceManager &SM = Ctx.getSourceManager();
+ SourceLocation Begin = If->getBeginLoc();
+ SourceLocation End = If->getEndLoc();
+ FileID BeginFile = SM.getFileID(Begin);
+ FileID EndFile = SM.getFileID(End);
+ if (BeginFile != EndFile)
+ return;
+ unsigned BeginLine = SM.getSpellingLineNumber(Begin);
+ unsigned EndLine = SM.getSpellingLineNumber(End);
+ if (BeginLine > EndLine)
+ return; // This case can't be good.
+ if ((EndLine - BeginLine) < Check.getLineCountThreshold())
+ return;
+ Check.diag(If->getBeginLoc(), "use early exit")
+ << createReverseConditionFix(If->getCond(), Ctx)
+ << FixItHint::CreateInsertion(If->getThen()->getBeginLoc(),
+ ("\n" + Continuation + ";\n").str())
+ << FixItHint::CreateRemoval(BodyCS->getLBracLoc())
+ << FixItHint::CreateRemoval(BodyCS->getRBracLoc());
+ checkBody<TerminationStmt>(BodyCS, Continuation);
+}
+} // namespace
+
+UseEarlyExitsCheck::UseEarlyExitsCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ LineCountThreshold(Options.get("LineCountThreshold", 10U)) {}
+
+void UseEarlyExitsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "LineCountThreshold", LineCountThreshold);
+}
+
+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
Index: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ 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<UseAnyOfAllOfCheck>(
"readability-use-anyofallof");
+ CheckFactories.registerCheck<UseEarlyExitsCheck>(
+ "readability-use-early-exits");
}
};
Index: clang-tools-extra/clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -48,6 +48,7 @@
UniqueptrDeleteReleaseCheck.cpp
UppercaseLiteralSuffixCheck.cpp
UseAnyOfAllOfCheck.cpp
+ UseEarlyExitsCheck.cpp
LINK_LIBS
clangTidy
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
