Add clang-tidy check to suggest replacement of conditional statement with std::min/std::max (#77816)

11happy · web-flow · commit c13e271a3836 · 2024-02-06T20:57:34.000+01:00
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)
diff --git 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
@@ -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<UseAnyOfAllOfCheck>(
         "readability-use-anyofallof");
+    CheckFactories.registerCheck<UseStdMinMaxCheck>(
+        "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
@@ -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("<algorithm>");
+
+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<TypedefType>(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<ElaboratedType>(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<IfStmt>("if");
+  const clang::LangOptions &LO = Result.Context->getLangOpts();
+  const auto *CondLhs = Result.Nodes.getNodeAs<Expr>("CondLhs");
+  const auto *CondRhs = Result.Nodes.getNodeAs<Expr>("CondRhs");
+  const auto *AssignLhs = Result.Nodes.getNodeAs<Expr>("AssignLhs");
+  const auto *AssignRhs = Result.Nodes.getNodeAs<Expr>("AssignRhs");
+  const auto *BinaryOp = Result.Nodes.getNodeAs<BinaryOperator>("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
@@ -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<TraversalKind> 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
@@ -100,6 +100,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`readability-use-std-min-max
+  <clang-tidy/checks/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
@@ -385,6 +385,7 @@ Clang-Tidy Checks
    :doc:`readability-uniqueptr-delete-release <readability/uniqueptr-delete-release>`, "Yes"
    :doc:`readability-uppercase-literal-suffix <readability/uppercase-literal-suffix>`, "Yes"
    :doc:`readability-use-anyofallof <readability/use-anyofallof>`,
+   :doc:`readability-use-std-min-max <readability/use-std-min-max>`, "Yes"
    :doc:`zircon-temporary-objects <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
@@ -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
