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 @@ -27,6 +27,7 @@ MisplacedArrayIndexCheck.cpp NamedParameterCheck.cpp NamespaceCommentCheck.cpp + NestedIfsCheck.cpp NonConstParameterCheck.cpp QualifiedAutoCheck.cpp ReadabilityTidyModule.cpp diff --git a/clang-tools-extra/clang-tidy/readability/NestedIfsCheck.h b/clang-tools-extra/clang-tidy/readability/NestedIfsCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/NestedIfsCheck.h @@ -0,0 +1,35 @@ +//===--- NestedIfsCheck.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_NESTEDIFSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NESTEDIFSCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// Detects nested `if` statements that could be merged into one by ANDing the +/// conditions. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability/nested-ifs.html +class NestedIfsCheck : public ClangTidyCheck { +public: + NestedIfsCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_NESTEDIFSCHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/NestedIfsCheck.cpp b/clang-tools-extra/clang-tidy/readability/NestedIfsCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/NestedIfsCheck.cpp @@ -0,0 +1,143 @@ +//===--- NestedIfsCheck.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 "NestedIfsCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Expr.h" +#include "clang/AST/ExprCXX.h" +#include "clang/AST/OperationKinds.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/Stmt.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/OperatorKinds.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +namespace { + +static bool needsWrapInParens(const Expr *E) { + if (const auto *BO = dyn_cast(E)) { + return BO->getOpcode() == BO_LOr || BO->isAssignmentOp() || BO->isCommaOp(); + } + if (const auto *OO = dyn_cast(E)) { + return OO->getNumArgs() == 2 && + (OO->isAssignmentOp() || OO->getOperator() == OO_PipePipe); + } + return isa(E); +} + +static StringRef joinConditions(bool LParen, bool RParen) { + static constexpr llvm::StringLiteral Join(") && ("); + return Join.slice(LParen ? 0 : 1, RParen ? Join.size() : Join.size() - 1); +} + +class NestedIfVisitor : public RecursiveASTVisitor { + using Base = RecursiveASTVisitor; + NestedIfsCheck &Check; + ASTContext &Ctx; + +public: + NestedIfVisitor(NestedIfsCheck &Check, ASTContext &Ctx) + : Check(Check), Ctx(Ctx) { + TraverseAST(Ctx); + } + + static bool isCanditateIf(const IfStmt *If, bool IsFirstConstexpr, + bool Outer = false) { + if (If->isConsteval()) + return false; + if (!Outer && IsFirstConstexpr != If->isConstexpr()) + return false; + if (If->hasElseStorage() || If->hasVarStorage()) + return false; + if (!If->getThen()) + return false; + // The first `if` in the chain can have an init statement, but any nested + // ones cannot. + return Outer || !If->hasInitStorage(); + } + + bool captureNested(IfStmt *If, SmallVectorImpl &Capture, + DataRecursionQueue *Queue) { + auto AllowConstexpr = If->isConstexpr(); + while (true) { + auto *Nested = dyn_cast(If->getThen()); + if (!Nested) { + if (auto *CS = dyn_cast(If->getThen())) { + if (CS->size() == 1) + Nested = dyn_cast(CS->body_front()); + } + } + if (!Nested || !isCanditateIf(Nested, AllowConstexpr)) + return Base::TraverseIfStmt(If, Queue); + + Capture.push_back(Nested); + If = Nested; + } + return true; + } + + bool TraverseIfStmt(IfStmt *If, DataRecursionQueue *Queue = nullptr) { + if (!isCanditateIf(If, false, true)) + return Base::TraverseIfStmt(If, Queue); + SmallVector Diagnose{If}; + auto Result = captureNested(If, Diagnose, Queue); + if (Diagnose.size() == 1) + return Result; + auto D = Check.diag(If->getBeginLoc(), "Nested ifs can be combined"); + for (const IfStmt *Item : Diagnose) + D << SourceRange(Item->getBeginLoc(), Item->getRParenLoc()); + SmallString<128> CondAppend; + bool PrevNeedsParens = needsWrapInParens(Diagnose.front()->getCond()); + if (PrevNeedsParens) + D << FixItHint::CreateInsertion(If->getCond()->getBeginLoc(), "("); + for (auto P = Diagnose.begin(), C = std::next(P), E = Diagnose.end(); + C != E; P = C++) { + const IfStmt *Prev = *P; + if (const auto *CS = dyn_cast(Prev->getThen())) + D << FixItHint::CreateRemoval(CS->getLBracLoc()) + << FixItHint::CreateRemoval(CS->getRBracLoc()); + const IfStmt *Cur = *C; + const bool CurNeedsParens = needsWrapInParens(Cur->getCond()); + CondAppend.append( + {joinConditions(PrevNeedsParens, CurNeedsParens), + Lexer::getSourceText( + CharSourceRange::getTokenRange(Cur->getCond()->getSourceRange()), + Ctx.getSourceManager(), Ctx.getLangOpts())}); + D << FixItHint::CreateRemoval(CharSourceRange::getTokenRange( + {Cur->getBeginLoc(), Cur->getRParenLoc()})); + PrevNeedsParens = CurNeedsParens; + } + if (PrevNeedsParens) + CondAppend.push_back(')'); + D << FixItHint::CreateInsertion(If->getRParenLoc(), CondAppend); + + return Result; + } +}; +} // namespace + +void NestedIfsCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(translationUnitDecl(), this); +} + +void NestedIfsCheck::check(const MatchFinder::MatchResult &Result) { + NestedIfVisitor(*this, *Result.Context); +} + +} // namespace readability +} // namespace tidy +} // namespace clang 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 @@ -31,6 +31,7 @@ #include "MisleadingIndentationCheck.h" #include "MisplacedArrayIndexCheck.h" #include "NamedParameterCheck.h" +#include "NestedIfsCheck.h" #include "NonConstParameterCheck.h" #include "QualifiedAutoCheck.h" #include "RedundantAccessSpecifiersCheck.h" @@ -101,6 +102,7 @@ "readability-misleading-indentation"); CheckFactories.registerCheck( "readability-misplaced-array-index"); + CheckFactories.registerCheck("readability-nested-ifs"); CheckFactories.registerCheck( "readability-qualified-auto"); CheckFactories.registerCheck( 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 @@ -99,6 +99,12 @@ New checks ^^^^^^^^^^ +- New :doc:`readability-nested-ifs + ` check. + + Detects nested ``if`` statements that could be merged into one by ANDing the + conditions. + 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 @@ -340,6 +340,7 @@ `readability-misleading-indentation `_, `readability-misplaced-array-index `_, "Yes" `readability-named-parameter `_, "Yes" + `readability-nested-ifs `_, "Yes" `readability-non-const-parameter `_, "Yes" `readability-qualified-auto `_, "Yes" `readability-redundant-access-specifiers `_, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/nested-ifs.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/nested-ifs.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/nested-ifs.rst @@ -0,0 +1,57 @@ +.. title:: clang-tidy - readability-nested-ifs + +readability-nested-ifs +====================== + +Detects nested ``if`` statements that could be merged into one by ANDing the +conditions. + +This transformation requires that the outer ``if`` contains a single ``if`` as +its then branch. Both ``if`` statements cannot have an ``else`` branch, +condition variable or have a ``consteval`` specifier. + +Example: + +.. code-block:: c++ + + void foo() { + if (Condition1) { + if (Condition2) { + Process(); + } + } + } + +Would be transformed to: + +.. code-block:: c++ + + void foo() { + if (Condition1 && Condition2) { + Process(); + } + } + +This will also recurse to enable merging `N` nested ifs. + +.. code-block:: c++ + + void foo() { + if (Condition1) { + if (Condition...) { + if (ConditionN) { + Process() + } + } + } + } + +Would be transformed to: + +.. code-block:: c++ + + void foo() { + if (Condition1 && Condition... && ConditionN) { + Process() + } + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/nested-ifs-cxx17.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/nested-ifs-cxx17.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/nested-ifs-cxx17.cpp @@ -0,0 +1,74 @@ +// RUN: %check_clang_tidy -std=c++17-or-later %s readability-nested-ifs %t -- -- -fno-delayed-template-parsing + +// Ensure the check handles if initializers and constexpr if statements +// correctly. + +void foo(); +void bar(); + +bool cond(int X = 0); + +void good() { + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined + if (bool X = cond(); X) { + if (cond()) { + if (cond(1)) + bar(); + } + } // End + // CHECK-FIXES: if (bool X = cond(); X && cond() && cond(1)) + // CHECK-FIXES-NEXT: {{ }} + // CHECK-FIXES-NEXT: {{ }} + // CHECK-FIXES-NEXT: bar(); + // CHECK-FIXES-NEXT: {{ }} + // CHECK-FIXES-NEXT: // End + + // The initializer doesn't have to be a variable declaration. + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined + if (foo(); true) { + if (cond()) + bar(); + } +} + +void bad() { + // Only the outermost `if` statement can have an initializer. + if (bool X = cond(); X) { + if (bool Y = cond(1); Y) { + bar(); + } + } + + if (cond()) { + if (bool X = cond(1); X) { + bar(); + } + } +} + +template void constexprIfs() { + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined + if constexpr (B) { + if constexpr (C) { + bar(); + } + } // End + // CHECK-FIXES: if constexpr (B && C) + // CHECK-FIXES-NEXT: { + // CHECK-FIXES-NEXT: bar(); + // CHECK-FIXES-NEXT: } + // CHECK-FIXES-NEXT: // End + + // constexpr ifs are only converted if both ifs are constexpr. + if constexpr (B) { + if (C) { + bar(); + } + } + + if (B) { + if constexpr (C) { + bar(); + } + } +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/nested-ifs.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/nested-ifs.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/nested-ifs.cpp @@ -0,0 +1,116 @@ +// RUN: %check_clang_tidy %s readability-nested-ifs %t + +void foo(); +void bar(); + +bool cond(int X = 0); + +void good() { + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined + if (cond()) { + if (cond(1)) { + foo(); + bar(); + } + } // End + // CHECK-FIXES: if (cond() && cond(1)) + // CHECK-FIXES-NEXT: { + // CHECK-FIXES-NEXT: foo(); + // CHECK-FIXES-NEXT: bar(); + // CHECK-FIXES-NEXT: } + // CHECK-FIXES-NEXT: // End + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: Nested ifs can be combined + // CHECK-MESSAGES: :[[@LINE+4]]:7: warning: Nested ifs can be combined + if (cond()) { + if (cond(1)) { + foo(); + if (cond(2)) { + if (cond(3)) { + bar(); + } + } + } + } // End + + // CHECK-FIXES: if (cond() && cond(1)) + // CHECK-FIXES-NEXT: { + // CHECK-FIXES-NEXT: foo(); + // CHECK-FIXES-NEXT: if (cond(2) && cond(3)) + // CHECK-FIXES-NEXT: { + // CHECK-FIXES-NEXT: bar(); + // CHECK-FIXES-NEXT: } + // CHECK-FIXES-NEXT: {{ }} + // CHECK-FIXES-NEXT: } + // CHECK-FIXES-NEXT: // End + + // CHECK-MESSAGES: :[[@LINE+2]]:5: warning: Nested ifs can be combined + if (bool B = cond()) { + if (cond(1)) { + if (cond(2)) { + foo(); + } + } + } // End + + // CHECK-FIXES: if (bool B = cond()) { + // CHECK-FIXES-NEXT: if (cond(1) && cond(2)) + // CHECK-FIXES-NEXT: { + // CHECK-FIXES-NEXT: foo(); + // CHECK-FIXES-NEXT: } + // CHECK-FIXES-NEXT: {{ }} + // CHECK-FIXES-NEXT: } // End + + + // Ensure parens are inserted when needed. + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined + if (cond() || cond(1)) if (cond(2)) {} + // CHECK-FIXES: if ((cond() || cond(1)) && cond(2)) {} + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined + if (cond()) if (cond(1), cond(2)) {} + // CHECK-FIXES: if (cond() && (cond(1), cond(2))) {} + + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Nested ifs can be combined + if (cond() || cond(1)) if (cond(2) ? cond(3) : cond(4)) {} + // CHECK-FIXES: if ((cond() || cond(1)) && (cond(2) ? cond(3) : cond(4))) {} +} + +void bad() { + // Condition variable on either nesting can't be converted. + if (bool B = cond()) + if (cond()) + foo(); + if (cond()) { + if (bool B = cond()) + bar(); + } + + // Can only have a single if statement as the body of the outer if. + if (cond()) { + foo(); + if (cond()) + bar(); + } + if (cond()) { + if (cond()) + foo(); + bar(); + } + + // Can't have an else statement on either if. + if (cond()) { + if (cond()) + foo(); + else + bar(); + } + + if (cond()) { + if (cond()) + foo(); + } else { + bar(); + } +}