diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index a6c8cbd8eb448..5728c9970fb65 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -55,6 +55,7 @@ add_clang_library(clangTidyReadabilityModule UniqueptrDeleteReleaseCheck.cpp UppercaseLiteralSuffixCheck.cpp UseAnyOfAllOfCheck.cpp + UseStdMinMaxCheck.cpp LINK_LIBS clangTidy diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index 87b299bf1ef1c..bca2c425111f6 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -58,6 +58,7 @@ #include "UniqueptrDeleteReleaseCheck.h" #include "UppercaseLiteralSuffixCheck.h" #include "UseAnyOfAllOfCheck.h" +#include "UseStdMinMaxCheck.h" namespace clang::tidy { namespace readability { @@ -163,6 +164,8 @@ class ReadabilityModule : public ClangTidyModule { "readability-uppercase-literal-suffix"); CheckFactories.registerCheck( "readability-use-anyofallof"); + CheckFactories.registerCheck( + "readability-use-std-min-max"); } }; diff --git a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp new file mode 100644 index 0000000000000..9c5c2f3939c99 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp @@ -0,0 +1,188 @@ +//===--- UseStdMinMaxCheck.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 "UseStdMinMaxCheck.h" +#include "../utils/ASTUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Preprocessor.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +namespace { + +// Ignore if statements that are inside macros. +AST_MATCHER(IfStmt, isIfInMacro) { + return Node.getIfLoc().isMacroID() || Node.getEndLoc().isMacroID(); +} + +} // namespace + +static const llvm::StringRef AlgorithmHeader(""); + +static bool minCondition(const BinaryOperator::Opcode Op, const Expr *CondLhs, + const Expr *CondRhs, const Expr *AssignLhs, + const Expr *AssignRhs, const ASTContext &Context) { + if ((Op == BO_LT || Op == BO_LE) && + (tidy::utils::areStatementsIdentical(CondLhs, AssignRhs, Context) && + tidy::utils::areStatementsIdentical(CondRhs, AssignLhs, Context))) + return true; + + if ((Op == BO_GT || Op == BO_GE) && + (tidy::utils::areStatementsIdentical(CondLhs, AssignLhs, Context) && + tidy::utils::areStatementsIdentical(CondRhs, AssignRhs, Context))) + return true; + + return false; +} + +static bool maxCondition(const BinaryOperator::Opcode Op, const Expr *CondLhs, + const Expr *CondRhs, const Expr *AssignLhs, + const Expr *AssignRhs, const ASTContext &Context) { + if ((Op == BO_LT || Op == BO_LE) && + (tidy::utils::areStatementsIdentical(CondLhs, AssignLhs, Context) && + tidy::utils::areStatementsIdentical(CondRhs, AssignRhs, Context))) + return true; + + if ((Op == BO_GT || Op == BO_GE) && + (tidy::utils::areStatementsIdentical(CondLhs, AssignRhs, Context) && + tidy::utils::areStatementsIdentical(CondRhs, AssignLhs, Context))) + return true; + + return false; +} + +QualType getNonTemplateAlias(QualType QT) { + while (true) { + // cast to a TypedefType + if (const TypedefType *TT = dyn_cast(QT)) { + // check if the typedef is a template and if it is dependent + if (!TT->getDecl()->getDescribedTemplate() && + !TT->getDecl()->getDeclContext()->isDependentContext()) + return QT; + QT = TT->getDecl()->getUnderlyingType(); + } + // cast to elaborated type + else if (const ElaboratedType *ET = dyn_cast(QT)) { + QT = ET->getNamedType(); + } else { + break; + } + } + return QT; +} + +static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs, + const Expr *AssignLhs, + const SourceManager &Source, + const LangOptions &LO, + StringRef FunctionName, + const BinaryOperator *BO) { + const llvm::StringRef CondLhsStr = Lexer::getSourceText( + Source.getExpansionRange(CondLhs->getSourceRange()), Source, LO); + const llvm::StringRef CondRhsStr = Lexer::getSourceText( + Source.getExpansionRange(CondRhs->getSourceRange()), Source, LO); + const llvm::StringRef AssignLhsStr = Lexer::getSourceText( + Source.getExpansionRange(AssignLhs->getSourceRange()), Source, LO); + + clang::QualType GlobalImplicitCastType; + clang::QualType LhsType = CondLhs->getType() + .getCanonicalType() + .getNonReferenceType() + .getUnqualifiedType(); + clang::QualType RhsType = CondRhs->getType() + .getCanonicalType() + .getNonReferenceType() + .getUnqualifiedType(); + if (LhsType != RhsType) { + GlobalImplicitCastType = getNonTemplateAlias(BO->getLHS()->getType()); + } + + return (AssignLhsStr + " = " + FunctionName + + (!GlobalImplicitCastType.isNull() + ? "<" + GlobalImplicitCastType.getAsString() + ">(" + : "(") + + CondLhsStr + ", " + CondRhsStr + ");") + .str(); +} + +UseStdMinMaxCheck::UseStdMinMaxCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IncludeInserter(Options.getLocalOrGlobal("IncludeStyle", + utils::IncludeSorter::IS_LLVM), + areDiagsSelfContained()) {} + +void UseStdMinMaxCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle()); +} + +void UseStdMinMaxCheck::registerMatchers(MatchFinder *Finder) { + auto AssignOperator = + binaryOperator(hasOperatorName("="), + hasLHS(expr(unless(isTypeDependent())).bind("AssignLhs")), + hasRHS(expr(unless(isTypeDependent())).bind("AssignRhs"))); + auto BinaryOperator = + binaryOperator(hasAnyOperatorName("<", ">", "<=", ">="), + hasLHS(expr(unless(isTypeDependent())).bind("CondLhs")), + hasRHS(expr(unless(isTypeDependent())).bind("CondRhs"))) + .bind("binaryOp"); + Finder->addMatcher( + ifStmt(stmt().bind("if"), unless(isIfInMacro()), + unless(hasElse(stmt())), // Ensure `if` has no `else` + hasCondition(BinaryOperator), + hasThen( + anyOf(stmt(AssignOperator), + compoundStmt(statementCountIs(1), has(AssignOperator)))), + hasParent(stmt(unless(ifStmt(hasElse( + equalsBoundNode("if"))))))), // Ensure `if` has no `else if` + this); +} + +void UseStdMinMaxCheck::registerPPCallbacks(const SourceManager &SM, + Preprocessor *PP, + Preprocessor *ModuleExpanderPP) { + IncludeInserter.registerPreprocessor(PP); +} + +void UseStdMinMaxCheck::check(const MatchFinder::MatchResult &Result) { + const auto *If = Result.Nodes.getNodeAs("if"); + const clang::LangOptions &LO = Result.Context->getLangOpts(); + const auto *CondLhs = Result.Nodes.getNodeAs("CondLhs"); + const auto *CondRhs = Result.Nodes.getNodeAs("CondRhs"); + const auto *AssignLhs = Result.Nodes.getNodeAs("AssignLhs"); + const auto *AssignRhs = Result.Nodes.getNodeAs("AssignRhs"); + const auto *BinaryOp = Result.Nodes.getNodeAs("binaryOp"); + const clang::BinaryOperatorKind BinaryOpcode = BinaryOp->getOpcode(); + const SourceLocation IfLocation = If->getIfLoc(); + const SourceLocation ThenLocation = If->getEndLoc(); + + auto ReplaceAndDiagnose = [&](const llvm::StringRef FunctionName) { + const SourceManager &Source = *Result.SourceManager; + diag(IfLocation, "use `%0` instead of `%1`") + << FunctionName << BinaryOp->getOpcodeStr() + << FixItHint::CreateReplacement( + SourceRange(IfLocation, Lexer::getLocForEndOfToken( + ThenLocation, 0, Source, LO)), + createReplacement(CondLhs, CondRhs, AssignLhs, Source, LO, + FunctionName, BinaryOp)) + << IncludeInserter.createIncludeInsertion( + Source.getFileID(If->getBeginLoc()), AlgorithmHeader); + }; + + if (minCondition(BinaryOpcode, CondLhs, CondRhs, AssignLhs, AssignRhs, + (*Result.Context))) { + ReplaceAndDiagnose("std::min"); + } else if (maxCondition(BinaryOpcode, CondLhs, CondRhs, AssignLhs, AssignRhs, + (*Result.Context))) { + ReplaceAndDiagnose("std::max"); + } +} + +} // namespace clang::tidy::readability diff --git a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.h b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.h new file mode 100644 index 0000000000000..b8d8b8c4fe894 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.h @@ -0,0 +1,42 @@ +//===--- UseStdMinMaxCheck.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_USESTDMINMAXCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USESTDMINMAXCHECK_H + +#include "../ClangTidyCheck.h" +#include "../utils/IncludeInserter.h" + +namespace clang::tidy::readability { + +/// Replaces certain conditional statements with equivalent calls to +/// ``std::min`` or ``std::max``. +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability/UseStdMinMax.html +class UseStdMinMaxCheck : public ClangTidyCheck { +public: + UseStdMinMaxCheck(StringRef Name, ClangTidyContext *Context); + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + std::optional getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } + +private: + utils::IncludeInserter IncludeInserter; +}; + +} // namespace clang::tidy::readability + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USESTDMINMAXCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 16309bd89bda4..e50914aed5f07 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -100,6 +100,12 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`readability-use-std-min-max + ` check. + + Replaces certain conditional statements with equivalent calls to + ``std::min`` or ``std::max``. + 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 index f40192ed9dea2..59ef69f390ee9 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -385,6 +385,7 @@ Clang-Tidy Checks :doc:`readability-uniqueptr-delete-release `, "Yes" :doc:`readability-uppercase-literal-suffix `, "Yes" :doc:`readability-use-anyofallof `, + :doc:`readability-use-std-min-max `, "Yes" :doc:`zircon-temporary-objects `, diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/use-std-min-max.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/use-std-min-max.rst new file mode 100644 index 0000000000000..73712bdba83b7 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/use-std-min-max.rst @@ -0,0 +1,29 @@ +.. title:: clang-tidy - readability-use-std-min-max + +readability-use-std-min-max +=========================== + +Replaces certain conditional statements with equivalent calls to +``std::min`` or ``std::max``. +Note: This may impact performance in critical code due to potential +additional stores compared to the original if statement. + +Before: + +.. code-block:: c++ + + void foo() { + int a = 2, b = 3; + if (a < b) + a = b; + } + + +After: + +.. code-block:: c++ + + void foo() { + int a = 2, b = 3; + a = std::max(a, b); + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp new file mode 100644 index 0000000000000..9c0e2eabda348 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp @@ -0,0 +1,254 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s readability-use-std-min-max %t -- -- -fno-delayed-template-parsing +#define MY_MACRO_MIN(a, b) ((a) < (b) ? (a) : (b)) + +constexpr int myConstexprMin(int a, int b) { + return a < b ? a : b; +} + +constexpr int myConstexprMax(int a, int b) { + return a > b ? a : b; +} + +#define MY_IF_MACRO(condition, statement) \ + if (condition) { \ + statement \ + } + +class MyClass { +public: + int member1; + int member2; +}; + +template + +void foo(T value7) { + int value1,value2,value3; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::max(value1, value2); + if (value1 < value2) + value1 = value2; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: value2 = std::min(value1, value2); + if (value1 < value2) + value2 = value1; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: value2 = std::min(value2, value1); + if (value2 > value1) + value2 = value1; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::max(value2, value1); + if (value2 > value1) + value1 = value2; + + // No suggestion needed here + if (value1 == value2) + value1 = value2; + + // CHECK-MESSAGES: :[[@LINE+3]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::max(value1, value4); + short value4; + if(value1` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::min(value1, myConstexprMax(value2, value3)); + if (value1 > myConstexprMax(value2, value3)) + value1 = myConstexprMax(value2, value3); + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<=` [readability-use-std-min-max] + // CHECK-FIXES: value2 = std::min(value1, value2); + if (value1 <= value2) + value2 = value1; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<=` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::max(value1, value2); + if (value1 <= value2) + value1 = value2; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>=` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::max(value2, value1); + if (value2 >= value1) + value1 = value2; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>=` [readability-use-std-min-max] + // CHECK-FIXES: value2 = std::min(value2, value1); + if (value2 >= value1) + value2 = value1; + + // CHECK-MESSAGES: :[[@LINE+3]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: obj.member1 = std::max(obj.member1, obj.member2); + MyClass obj; + if (obj.member1 < obj.member2) + obj.member1 = obj.member2; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: obj.member2 = std::min(obj.member1, obj.member2); + if (obj.member1 < obj.member2) + obj.member2 = obj.member1; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: obj.member2 = std::min(obj.member2, obj.member1); + if (obj.member2 > obj.member1) + obj.member2 = obj.member1; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: obj.member1 = std::max(obj.member2, obj.member1); + if (obj.member2 > obj.member1) + obj.member1 = obj.member2; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: obj.member1 = std::max(obj.member1, value4); + if (obj.member1 < value4) + obj.member1 = value4; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: value3 = std::min(obj.member1 + value2, value3); + if (obj.member1 + value2 < value3) + value3 = obj.member1 + value2; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<=` [readability-use-std-min-max] + // CHECK-FIXES: obj.member2 = std::min(value1, obj.member2); + if (value1 <= obj.member2) + obj.member2 = value1; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<=` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::max(value1, obj.member2); + if (value1 <= obj.member2) + value1 = obj.member2; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>=` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::max(obj.member2, value1); + if (obj.member2 >= value1) + value1 = obj.member2; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>=` [readability-use-std-min-max] + // CHECK-FIXES: obj.member2 = std::min(obj.member2, value1); + if (obj.member2 >= value1) + obj.member2 = value1; + + // No suggestion needed here + if (MY_MACRO_MIN(value1, value2) < value3) + value3 = MY_MACRO_MIN(value1, value2); + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: value4 = std::max(value4, value2); + if (value4 < value2){ + value4 = value2; + } + + // No suggestion needed here + if(value1 < value2) + value2 = value1; + else + value2 = value3; + + // No suggestion needed here + if(value1value2){ + value2 = value1; + } + + // CHECK-MESSAGES: :[[@LINE+3]]:5: warning: use `std::max` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: value1 = std::max(value1, value3); + if(value1 == value2){ + if(value1(value1, value4); + if(value1 == value2){ + if(value2 == value3){ + value3+=1; + if(value1value2){ + value2 = value3; + } + } + + // CHECK-MESSAGES: :[[@LINE+4]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: value6 = std::min(value5, value6); + unsigned int value5; + unsigned char value6; + if(value5 +struct MyVector +{ + using size_type = my_size; + size_type size() const; +}; + +void testVectorSizeType() { + MyVector v; + unsigned int value; + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: value = std::max(v.size(), value); + if (v.size() > value) + value = v.size(); + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] + // CHECK-FIXES: value = std::max(value, v.size()); + if (value < v.size()) + value = v.size(); +}