https://github.com/11happy created https://github.com/llvm/llvm-project/pull/77816
**Overview:** This pull request fixes #64914 where author suggests adding a readability check to propose the replacement of conditional statements with std::min/std::max for improved code readability. Additionally, reference is made to PyLint's similar checks: [consider-using-min-builtin](https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/consider-using-min-builtin.html) and [consider-using-max-builtin](https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/consider-using-max-builtin.html) **Testing:** - Tested the updated code. - Verified that other functionalities remain unaffected.  **Dependencies:** - No dependencies on other pull requests. **CC:** - @AaronBallman , @EugeneZelenko , @PiotrZSL , @carlosgalvezp >From 1883d987b2f83adaef05fdb47ae25c7b06582a64 Mon Sep 17 00:00:00 2001 From: 11happy <soni5ha...@gmail.com> Date: Fri, 12 Jan 2024 00:02:46 +0530 Subject: [PATCH 1/2] Add readability check to suggest replacement of conditional statement with std::min/std::max Signed-off-by: 11happy <soni5ha...@gmail.com> --- .../clang-tidy/readability/CMakeLists.txt | 1 + .../ConditionaltostdminmaxCheck.cpp | 86 +++++++++++++++++++ .../readability/ConditionaltostdminmaxCheck.h | 30 +++++++ .../readability/ReadabilityTidyModule.cpp | 3 + clang-tools-extra/docs/ReleaseNotes.rst | 6 ++ .../docs/clang-tidy/checks/list.rst | 1 + .../readability/ConditionalToStdMinMax.rst | 29 +++++++ .../readability/ConditionalToStdMinMax.cpp | 27 ++++++ 8 files changed, 183 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/readability/ConditionalToStdMinMax.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index 408c822b861c5f..4bc373bb69bb84 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -8,6 +8,7 @@ add_clang_library(clangTidyReadabilityModule AvoidReturnWithVoidValueCheck.cpp AvoidUnconditionalPreprocessorIfCheck.cpp BracesAroundStatementsCheck.cpp + ConditionaltostdminmaxCheck.cpp ConstReturnTypeCheck.cpp ContainerContainsCheck.cpp ContainerDataPointerCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp new file mode 100644 index 00000000000000..fba8c68f737450 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp @@ -0,0 +1,86 @@ +//===--- ConditionaltostdminmaxCheck.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 "ConditionaltostdminmaxCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +void ConditionaltostdminmaxCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + ifStmt( + has( + binaryOperator( + anyOf(hasOperatorName("<"),hasOperatorName(">")), + hasLHS(ignoringImpCasts(declRefExpr().bind("lhsVar1"))), + hasRHS(ignoringImpCasts(declRefExpr().bind("rhsVar1"))) + ) + ) + , + hasThen( + stmt( + binaryOperator( + hasOperatorName("="), + hasLHS(ignoringImpCasts(declRefExpr().bind("lhsVar2"))), + hasRHS(ignoringImpCasts(declRefExpr().bind("rhsVar2"))) + ) + ) + ) + ).bind("ifStmt"),this); + + + +} + +void ConditionaltostdminmaxCheck::check(const MatchFinder::MatchResult &Result) { + const DeclRefExpr *lhsVar1 = Result.Nodes.getNodeAs<DeclRefExpr>("lhsVar1"); + const DeclRefExpr *rhsVar1 = Result.Nodes.getNodeAs<DeclRefExpr>("rhsVar1"); + const DeclRefExpr *lhsVar2 = Result.Nodes.getNodeAs<DeclRefExpr>("lhsVar2"); + const DeclRefExpr *rhsVar2 = Result.Nodes.getNodeAs<DeclRefExpr>("rhsVar2"); + const IfStmt *ifStmt = Result.Nodes.getNodeAs<IfStmt>("ifStmt"); + + if (!lhsVar1 || !rhsVar1 || !lhsVar2 || !rhsVar2 || !ifStmt) + return; + + const BinaryOperator *binaryOp = dyn_cast<BinaryOperator>(ifStmt->getCond()); + if (!binaryOp) + return; + + SourceLocation ifLocation = ifStmt->getIfLoc(); + SourceLocation thenLocation = ifStmt->getEndLoc(); + + if(binaryOp->getOpcode() == BO_LT){ + if(lhsVar1->getDecl() == lhsVar2->getDecl() && rhsVar1->getDecl() == rhsVar2->getDecl()){ + diag(ifStmt->getIfLoc(), "use std::max instead of <")<<FixItHint::CreateReplacement(SourceRange(ifLocation, thenLocation), + lhsVar2->getNameInfo().getAsString() + " = std::max(" + lhsVar1->getNameInfo().getAsString() + ", " + + rhsVar1->getNameInfo().getAsString() + ")"); + } + else if(lhsVar1->getDecl() == rhsVar2->getDecl() && rhsVar1->getDecl() == lhsVar2->getDecl()){ + diag(ifStmt->getIfLoc(), "use std::min instead of <")<<FixItHint::CreateReplacement(SourceRange(ifLocation, thenLocation), + lhsVar2->getNameInfo().getAsString() + " = std::min(" + lhsVar1->getNameInfo().getAsString() + ", " + + rhsVar1->getNameInfo().getAsString() + ")"); + } + } + else if(binaryOp->getOpcode() == BO_GT){ + if(lhsVar1->getDecl() == lhsVar2->getDecl() && rhsVar1->getDecl() == rhsVar2->getDecl()){ + diag(ifStmt->getIfLoc(), "use std::min instead of >")<<FixItHint::CreateReplacement(SourceRange(ifLocation, thenLocation), + lhsVar2->getNameInfo().getAsString() + " = std::min(" + lhsVar1->getNameInfo().getAsString() + ", " + + rhsVar1->getNameInfo().getAsString() + ")"); + } + else if(lhsVar1->getDecl() == rhsVar2->getDecl() && rhsVar1->getDecl() == lhsVar2->getDecl()){ + diag(ifStmt->getIfLoc(), "use std::max instead of >")<<FixItHint::CreateReplacement(SourceRange(ifLocation, thenLocation), + lhsVar2->getNameInfo().getAsString() + " = std::max(" + lhsVar1->getNameInfo().getAsString() + ", " + + rhsVar1->getNameInfo().getAsString() + ")"); + } + } +} + +} // namespace clang::tidy::readability diff --git a/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h new file mode 100644 index 00000000000000..b7eabcc7d16d41 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h @@ -0,0 +1,30 @@ +//===--- ConditionaltostdminmaxCheck.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_CONDITIONALTOSTDMINMAXCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONDITIONALTOSTDMINMAXCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::readability { + +/// FIXME: replaces certain conditional statements with equivalent std::min or std::max expressions, improving readability and promoting the use of standard library functions." +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability/ConditionalToStdMinMax.html +class ConditionaltostdminmaxCheck : public ClangTidyCheck { +public: + ConditionaltostdminmaxCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::readability + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONDITIONALTOSTDMINMAXCHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index 0b0aad7c0dcb36..63f8f03be6bb91 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -13,6 +13,7 @@ #include "AvoidReturnWithVoidValueCheck.h" #include "AvoidUnconditionalPreprocessorIfCheck.h" #include "BracesAroundStatementsCheck.h" +#include "ConditionaltostdminmaxCheck.h" #include "ConstReturnTypeCheck.h" #include "ContainerContainsCheck.h" #include "ContainerDataPointerCheck.h" @@ -62,6 +63,8 @@ namespace readability { class ReadabilityModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck<ConditionaltostdminmaxCheck>( + "readability-ConditionalToStdMinMax"); CheckFactories.registerCheck<AvoidConstParamsInDecls>( "readability-avoid-const-params-in-decls"); CheckFactories.registerCheck<AvoidReturnWithVoidValueCheck>( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index b4d87e0ed2a67a..c3fb990ad6a738 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -224,6 +224,12 @@ New checks Recommends the smallest possible underlying type for an ``enum`` or ``enum`` class based on the range of its enumerators. +- New :doc:`readability-ConditionalToStdMinMax + <clang-tidy/checks/readability/ConditionalToStdMinMax>` check. + + Replaces certain conditional statements with equivalent std::min or std::max expressions, + improving readability and promoting the use of standard library functions. + - New :doc:`readability-reference-to-constructed-temporary <clang-tidy/checks/readability/reference-to-constructed-temporary>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2f86121ad87299..c2eac55425c69e 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -336,6 +336,7 @@ Clang-Tidy Checks :doc:`portability-restrict-system-includes <portability/restrict-system-includes>`, "Yes" :doc:`portability-simd-intrinsics <portability/simd-intrinsics>`, :doc:`portability-std-allocator-const <portability/std-allocator-const>`, + :doc:`readability-ConditionalToStdMinMax <readability/ConditionalToStdMinMax>`, "Yes" :doc:`readability-avoid-const-params-in-decls <readability/avoid-const-params-in-decls>`, "Yes" :doc:`readability-avoid-return-with-void-value <readability/avoid-return-with-void-value>`, :doc:`readability-avoid-unconditional-preprocessor-if <readability/avoid-unconditional-preprocessor-if>`, diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/ConditionalToStdMinMax.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/ConditionalToStdMinMax.rst new file mode 100644 index 00000000000000..e95858999b2772 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/ConditionalToStdMinMax.rst @@ -0,0 +1,29 @@ +.. title:: clang-tidy - readability-ConditionalToStdMinMax + +readability-ConditionalToStdMinMax +================================== + +Replaces certain conditional statements with equivalent std::min or std::max expressions, +improving readability and promoting the use of standard library functions. + +Examples: + +Before: + +.. code-block:: c++ + + void foo(){ + int a,b; + if(a < b) + a = b; + } + + +After: + +.. code-block:: c++ + + int main(){ + a = std::max(a, b); + + } \ No newline at end of file diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp new file mode 100644 index 00000000000000..d29e9aa9fec708 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp @@ -0,0 +1,27 @@ +// RUN: %check_clang_tidy %s readability-ConditionalToStdMinMax %t + +void foo() { + int value1,value2; + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: use std::max instead of < [readability-ConditionalToStdMinMax] + if (value1 < value2) + value1 = value2; // CHECK-FIXES: value1 = std::max(value1, value2); + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: use std::min instead of < [readability-ConditionalToStdMinMax] + if (value1 < value2) + value2 = value1; // CHECK-FIXES: value2 = std::min(value1, value2); + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: use std::min instead of > [readability-ConditionalToStdMinMax] + if (value2 > value1) + value2 = value1; // CHECK-FIXES: value2 = std::min(value2, value1); + + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: use std::max instead of > [readability-ConditionalToStdMinMax] + if (value2 > value1) + value1 = value2; // CHECK-FIXES: value1 = std::max(value2, value1); + + // No suggestion needed here + if (value1 == value2) + value1 = value2; + + +} \ No newline at end of file >From 89be4baf2591918b11b633d2430050dc41068fde Mon Sep 17 00:00:00 2001 From: 11happy <soni5ha...@gmail.com> Date: Fri, 12 Jan 2024 00:03:16 +0530 Subject: [PATCH 2/2] Formatted the code Signed-off-by: 11happy <soni5ha...@gmail.com> --- .../ConditionaltostdminmaxCheck.cpp | 96 ++++++++++--------- .../readability/ConditionaltostdminmaxCheck.h | 4 +- 2 files changed, 52 insertions(+), 48 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp index fba8c68f737450..86420765d6f6c6 100644 --- a/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp @@ -16,69 +16,71 @@ namespace clang::tidy::readability { void ConditionaltostdminmaxCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( - ifStmt( - has( - binaryOperator( - anyOf(hasOperatorName("<"),hasOperatorName(">")), - hasLHS(ignoringImpCasts(declRefExpr().bind("lhsVar1"))), - hasRHS(ignoringImpCasts(declRefExpr().bind("rhsVar1"))) - ) - ) - , - hasThen( - stmt( - binaryOperator( - hasOperatorName("="), - hasLHS(ignoringImpCasts(declRefExpr().bind("lhsVar2"))), - hasRHS(ignoringImpCasts(declRefExpr().bind("rhsVar2"))) - ) - ) - ) - ).bind("ifStmt"),this); - - - + ifStmt(has(binaryOperator( + anyOf(hasOperatorName("<"), hasOperatorName(">")), + hasLHS(ignoringImpCasts(declRefExpr().bind("lhsVar1"))), + hasRHS(ignoringImpCasts(declRefExpr().bind("rhsVar1"))))), + hasThen(stmt(binaryOperator( + hasOperatorName("="), + hasLHS(ignoringImpCasts(declRefExpr().bind("lhsVar2"))), + hasRHS(ignoringImpCasts(declRefExpr().bind("rhsVar2"))))))) + .bind("ifStmt"), + this); } -void ConditionaltostdminmaxCheck::check(const MatchFinder::MatchResult &Result) { +void ConditionaltostdminmaxCheck::check( + const MatchFinder::MatchResult &Result) { const DeclRefExpr *lhsVar1 = Result.Nodes.getNodeAs<DeclRefExpr>("lhsVar1"); const DeclRefExpr *rhsVar1 = Result.Nodes.getNodeAs<DeclRefExpr>("rhsVar1"); const DeclRefExpr *lhsVar2 = Result.Nodes.getNodeAs<DeclRefExpr>("lhsVar2"); - const DeclRefExpr *rhsVar2 = Result.Nodes.getNodeAs<DeclRefExpr>("rhsVar2"); + const DeclRefExpr *rhsVar2 = Result.Nodes.getNodeAs<DeclRefExpr>("rhsVar2"); const IfStmt *ifStmt = Result.Nodes.getNodeAs<IfStmt>("ifStmt"); if (!lhsVar1 || !rhsVar1 || !lhsVar2 || !rhsVar2 || !ifStmt) - return; + return; const BinaryOperator *binaryOp = dyn_cast<BinaryOperator>(ifStmt->getCond()); if (!binaryOp) - return; + return; SourceLocation ifLocation = ifStmt->getIfLoc(); SourceLocation thenLocation = ifStmt->getEndLoc(); - if(binaryOp->getOpcode() == BO_LT){ - if(lhsVar1->getDecl() == lhsVar2->getDecl() && rhsVar1->getDecl() == rhsVar2->getDecl()){ - diag(ifStmt->getIfLoc(), "use std::max instead of <")<<FixItHint::CreateReplacement(SourceRange(ifLocation, thenLocation), - lhsVar2->getNameInfo().getAsString() + " = std::max(" + lhsVar1->getNameInfo().getAsString() + ", " + - rhsVar1->getNameInfo().getAsString() + ")"); - } - else if(lhsVar1->getDecl() == rhsVar2->getDecl() && rhsVar1->getDecl() == lhsVar2->getDecl()){ - diag(ifStmt->getIfLoc(), "use std::min instead of <")<<FixItHint::CreateReplacement(SourceRange(ifLocation, thenLocation), - lhsVar2->getNameInfo().getAsString() + " = std::min(" + lhsVar1->getNameInfo().getAsString() + ", " + - rhsVar1->getNameInfo().getAsString() + ")"); - } - } - else if(binaryOp->getOpcode() == BO_GT){ - if(lhsVar1->getDecl() == lhsVar2->getDecl() && rhsVar1->getDecl() == rhsVar2->getDecl()){ - diag(ifStmt->getIfLoc(), "use std::min instead of >")<<FixItHint::CreateReplacement(SourceRange(ifLocation, thenLocation), - lhsVar2->getNameInfo().getAsString() + " = std::min(" + lhsVar1->getNameInfo().getAsString() + ", " + - rhsVar1->getNameInfo().getAsString() + ")"); + if (binaryOp->getOpcode() == BO_LT) { + if (lhsVar1->getDecl() == lhsVar2->getDecl() && + rhsVar1->getDecl() == rhsVar2->getDecl()) { + diag(ifStmt->getIfLoc(), "use std::max instead of <") + << FixItHint::CreateReplacement( + SourceRange(ifLocation, thenLocation), + lhsVar2->getNameInfo().getAsString() + " = std::max(" + + lhsVar1->getNameInfo().getAsString() + ", " + + rhsVar1->getNameInfo().getAsString() + ")"); + } else if (lhsVar1->getDecl() == rhsVar2->getDecl() && + rhsVar1->getDecl() == lhsVar2->getDecl()) { + diag(ifStmt->getIfLoc(), "use std::min instead of <") + << FixItHint::CreateReplacement( + SourceRange(ifLocation, thenLocation), + lhsVar2->getNameInfo().getAsString() + " = std::min(" + + lhsVar1->getNameInfo().getAsString() + ", " + + rhsVar1->getNameInfo().getAsString() + ")"); } - else if(lhsVar1->getDecl() == rhsVar2->getDecl() && rhsVar1->getDecl() == lhsVar2->getDecl()){ - diag(ifStmt->getIfLoc(), "use std::max instead of >")<<FixItHint::CreateReplacement(SourceRange(ifLocation, thenLocation), - lhsVar2->getNameInfo().getAsString() + " = std::max(" + lhsVar1->getNameInfo().getAsString() + ", " + - rhsVar1->getNameInfo().getAsString() + ")"); + } else if (binaryOp->getOpcode() == BO_GT) { + if (lhsVar1->getDecl() == lhsVar2->getDecl() && + rhsVar1->getDecl() == rhsVar2->getDecl()) { + diag(ifStmt->getIfLoc(), "use std::min instead of >") + << FixItHint::CreateReplacement( + SourceRange(ifLocation, thenLocation), + lhsVar2->getNameInfo().getAsString() + " = std::min(" + + lhsVar1->getNameInfo().getAsString() + ", " + + rhsVar1->getNameInfo().getAsString() + ")"); + } else if (lhsVar1->getDecl() == rhsVar2->getDecl() && + rhsVar1->getDecl() == lhsVar2->getDecl()) { + diag(ifStmt->getIfLoc(), "use std::max instead of >") + << FixItHint::CreateReplacement( + SourceRange(ifLocation, thenLocation), + lhsVar2->getNameInfo().getAsString() + " = std::max(" + + lhsVar1->getNameInfo().getAsString() + ", " + + rhsVar1->getNameInfo().getAsString() + ")"); } } } diff --git a/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h index b7eabcc7d16d41..02ebed83fed6c8 100644 --- a/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h +++ b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h @@ -13,7 +13,9 @@ namespace clang::tidy::readability { -/// FIXME: replaces certain conditional statements with equivalent std::min or std::max expressions, improving readability and promoting the use of standard library functions." +/// FIXME: replaces certain conditional statements with equivalent std::min or +/// std::max expressions, improving readability and promoting the use of +/// standard library functions." /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/readability/ConditionalToStdMinMax.html _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
