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

11happy · 2024-01-11T18:41:59Z

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 and consider-using-max-builtin

Testing:

Tested the updated code.
Verified that other functionalities remain unaffected.

Dependencies:

No dependencies on other pull requests.

CC:

@AaronBallman , @EugeneZelenko , @PiotrZSL , @carlosgalvezp

… with std::min/std::max Signed-off-by: 11happy <[email protected]>

Signed-off-by: 11happy <[email protected]>

llvmbot · 2024-01-11T18:42:30Z

@llvm/pr-subscribers-clang-tidy

Author: Bhuminjay Soni (11happy)

Changes

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 and consider-using-max-builtin

Testing:

Tested the updated code.
Verified that other functionalities remain unaffected.

Dependencies:

No dependencies on other pull requests.

CC:

@AaronBallman , @EugeneZelenko , @PiotrZSL , @carlosgalvezp

Full diff: https://github.com/llvm/llvm-project/pull/77816.diff

8 Files Affected:

(modified) clang-tools-extra/clang-tidy/readability/CMakeLists.txt (+1)
(added) clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp (+88)
(added) clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h (+32)
(modified) clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp (+3)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
(modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
(added) clang-tools-extra/docs/clang-tidy/checks/readability/ConditionalToStdMinMax.rst (+29)
(added) clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp (+27)

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..86420765d6f6c6
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp
@@ -0,0 +1,88 @@
+//===--- 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..02ebed83fed6c8
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h
@@ -0,0 +1,32 @@
+//===--- 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

Signed-off-by: 11happy <[email protected]>

EugeneZelenko

Check should also add #include if it's not included already.

clang-tools-extra/clang-tidy/readability/CMakeLists.txt

clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp

clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.h

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/docs/clang-tidy/checks/readability/ConditionalToStdMinMax.rst

clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp

PiotrZSL

Some tweaks needed. Note that there is also issue for:

std::max(a, std::max(b, c)) -> std::max({a, b, c})

and this check would be nice thing for that also, but not under this PR.

Overall you got something that works, but is very strict, now I would like to see it to become more .. generic and could land.

clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp

clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp

clang-tools-extra/docs/clang-tidy/checks/readability/ConditionalToStdMinMax.rst

clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/readability/ConditionalToStdMinMax.rst

clang-tools-extra/clang-tidy/readability/ConditionaltostdminmaxCheck.cpp

11happy · 2024-01-11T19:23:40Z

Thank you for the suggestions and your guidance I will update it as soon as possible.

clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp

5chmidti · 2024-01-12T00:17:03Z

LLVM uses CamelCase instead of camelCase for variable names. This is enforced by the top-level .clang-tidy file so you don't need to do this by hand, run clang-tidy and let it apply the fix-its on your file (clangd should provide these fixes as well, but currently only per var, not all at once).

This reverts commit e1b65a9.

This reverts commit 89be4ba.

…tatement with std::min/std::max" This reverts commit 1883d98.

Signed-off-by: 11happy <[email protected]>

11happy · 2024-01-12T11:59:01Z

Thank you for all the suggestions , I think I have tried to incorporate all of them .

Signed-off-by: 11happy <[email protected]>

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/readability/use-std-min-max.rst

clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp

Signed-off-by: 11happy <[email protected]>

clang-tools-extra/docs/clang-tidy/checks/readability/use-std-min-max.rst

Signed-off-by: 11happy <[email protected]>

11happy · 2024-01-13T07:00:38Z

@PiotrZSL can you please review the changes.
thank you.

clang-tools-extra/docs/clang-tidy/checks/readability/use-std-min-max.rst

clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp

clang-tools-extra/docs/clang-tidy/checks/readability/use-std-min-max.rst

Signed-off-by: 11happy <[email protected]>

11happy · 2024-01-13T15:50:08Z

I have added the initialisation of variables and also added tests involving constexpr,functions etc.

clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp

Signed-off-by: 11happy <[email protected]>

PiotrZSL

Too overengineered. There are easier ways, just check comments.
Except that, looks +- fine.

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp

Signed-off-by: 11happy <[email protected]>

PiotrZSL

Other thing is that reviewer should mark issues as resolved after checking with code, not author. Otherwise is hard to figure out what's done and what's not. Adding some comment like "Done" is sufficient usually unless issues are simple.

Next thing regarding your 2 comments that somehow cannot find in GitHub and exist only in my mail box:

vector size

  std::vector<std::vector<Foo>> a;
    unsigned int b = a[0].size();
    if(a[0].size() > b)
        b = a[0].size();

first, you do not have a test for that, like:

using my_size = unsigned long long;

template<typename T>
struct MyVector
{
    using size_type = my_size;
    size_type size() const;
};

void testVectorSizeType() {
  MyVector<int> v;

  unsigned int value;
  if (v.size() > value)
    value = v.size();

  if (value < v.size())
    value = v.size();
}

Second, in this case check in current state work fine and will put unsigned long long as a type, the only problem is that unsigned long long is ugly.

ElaboratedType 0x55cfafb2f390 'size_type' sugar
`-TypedefType 0x55cfafb2f360 'MyVector::size_type' sugar
  |-TypeAlias 0x55cfafb2f300 'size_type'
  `-ElaboratedType 0x55cfafb2f2c0 'my_size' sugar
    `-TypedefType 0x55cfafb2f290 'my_size' sugar
      |-TypeAlias 0x55cfafb2d900 'my_size'
      `-BuiltinType 0x55cfafabb710 'unsigned long long'

This is multi level typedef, proper solution would be to drop elaborated type until we get into alias that isn't template instance and isn't in template instance and isn't in template..
You can look into readability-redundant-casting how to drop some levels of elaborated types. And use that instead of getCanonicalType.

value1 + value2 < valuex
You should check only that binary operator that check trigger:

IfStmt 0x56422decb568 <line:256:3, line:257:23>
      |-BinaryOperator 0x56422decb430 <line:256:6, col:24> 'bool' '<'
      | |-ImplicitCastExpr 0x56422decb418 <col:6, col:15> 'unsigned int' <IntegralCast>
      | | `-BinaryOperator 0x56422decb3c0 <col:6, col:15> 'int' '+'
      | |   |-ImplicitCastExpr 0x56422decb378 <col:6> 'int' <IntegralCast>
      | |   | `-ImplicitCastExpr 0x56422decb360 <col:6> 'unsigned char' <LValueToRValue>
      | |   |   `-DeclRefExpr 0x56422decb320 <col:6> 'unsigned char' lvalue Var 0x56422decb0c8 'value1' 'unsigned char'
      | |   `-ImplicitCastExpr 0x56422decb3a8 <col:15> 'int' <IntegralCast>
      | |     `-ImplicitCastExpr 0x56422decb390 <col:15> 'short' <LValueToRValue>
      | |       `-DeclRefExpr 0x56422decb340 <col:15> 'short' lvalue Var 0x56422decb198 'value2' 'short'
      | `-ImplicitCastExpr 0x56422decb400 <col:24> 'unsigned int' <LValueToRValue>
      |   `-DeclRefExpr 0x56422decb3e0 <col:24> 'unsigned int' lvalue Var 0x56422decb268 'valuex' 'unsigned int'
      `-BinaryOperator 0x56422decb548 <line:257:5, col:23> 'unsigned int' lvalue '='
        |-DeclRefExpr 0x56422decb450 <col:5> 'unsigned int' lvalue Var 0x56422decb268 'valuex' 'unsigned int'
        `-ImplicitCastExpr 0x56422decb530 <col:14, col:23> 'unsigned int' <IntegralCast>
          `-BinaryOperator 0x56422decb510 <col:14, col:23> 'int' '+'
            |-ImplicitCastExpr 0x56422decb4c8 <col:14> 'int' <IntegralCast>
            | `-ImplicitCastExpr 0x56422decb4b0 <col:14> 'unsigned char' <LValueToRValue>
            |   `-DeclRefExpr 0x56422decb470 <col:14> 'unsigned char' lvalue Var 0x56422decb0c8 'value1' 'unsigned char'
            `-ImplicitCastExpr 0x56422decb4f8 <col:23> 'int' <IntegralCast>
              `-ImplicitCastExpr 0x56422decb4e0 <col:23> 'short' <LValueToRValue>
                `-DeclRefExpr 0x56422decb490 <col:23> 'short' lvalue Var 0x56422decb198 'value2' 'short'

in this case we got: unsigned char + short < unsigned int, first part will be cased to unsigned int even if ``unsigned char + shortgives int, and that's fine. Current version of check handles that well,GlobalImplicitCastType = BO->getLHS()->getType();` does a trick.

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp

Signed-off-by: 11happy <[email protected]>

11happy · 2024-01-30T09:20:45Z

Other thing is that reviewer should mark issues as resolved after checking with code, not author. Otherwise is hard to figure out what's done and what's not. Adding some comment like "Done" is sufficient usually unless issues are simple.

Next thing regarding your 2 comments that somehow cannot find in GitHub and exist only in my mail box:

vector size
  std::vector<std::vector<Foo>> a;
    unsigned int b = a[0].size();
    if(a[0].size() > b)
        b = a[0].size();
first, you do not have a test for that, like:
using my_size = unsigned long long;

template<typename T>
struct MyVector
{
    using size_type = my_size;
    size_type size() const;
};

void testVectorSizeType() {
  MyVector<int> v;

  unsigned int value;
  if (v.size() > value)
    value = v.size();

  if (value < v.size())
    value = v.size();
}
Second, in this case check in current state work fine and will put unsigned long long as a type, the only problem is that unsigned long long is ugly.
ElaboratedType 0x55cfafb2f390 'size_type' sugar
`-TypedefType 0x55cfafb2f360 'MyVector::size_type' sugar
  |-TypeAlias 0x55cfafb2f300 'size_type'
  `-ElaboratedType 0x55cfafb2f2c0 'my_size' sugar
    `-TypedefType 0x55cfafb2f290 'my_size' sugar
      |-TypeAlias 0x55cfafb2d900 'my_size'
      `-BuiltinType 0x55cfafabb710 'unsigned long long'
This is multi level typedef, proper solution would be to drop elaborated type until we get into alias that isn't template instance and isn't in template instance and isn't in template.. You can look into readability-redundant-casting how to drop some levels of elaborated types. And use that instead of getCanonicalType.

@PiotrZSL I have attempted to implement this suggestion, but I am encountering difficulties and confusion due to the various types involved. Could you kindly provide some guidance or assistance on how to effectively simplify the type hierarchy in this context? I have explored methods like getDesugaredType() but would appreciate your guidance specifically

Signed-off-by: 11happy <[email protected]>

11happy · 2024-02-01T10:16:41Z

@PiotrZSL , I have implemented the solution you suggested, took me some time though , but I can say it got me better understanding of how things work. Thank you

11happy · 2024-02-04T18:24:18Z

Humble Ping! (as per the rules ping after week of inactivity) @PiotrZSL , @5chmidti , @felix642 , @EugeneZelenko , I think its nearly done.

PiotrZSL

LGTM.
The only thing that I'm still thinking is if std is needed in name, and maybe it should be like just use-min-max.

felix642

LGTM,

There is still some work to do to support more cases like ternary operators and if/else statements, but I think we have a decent foundation to work on and we can add these cases later in other PRs.

5chmidti

LGTM, just two small nits.

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp

Signed-off-by: 11happy <[email protected]>

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.h

Signed-off-by: 11happy <[email protected]>

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp

Signed-off-by: 11happy <[email protected]>

PiotrZSL · 2024-02-05T21:33:55Z

Unless you want this to be merged as "[email protected]", change your email privacy settings.

11happy · 2024-02-06T02:38:53Z

Unless you want this to be merged as "[email protected]", change your email privacy settings.

I have changed the settings.

11happy added 2 commits January 12, 2024 00:02

Add readability check to suggest replacement of conditional statement…

1883d98

… with std::min/std::max Signed-off-by: 11happy <[email protected]>

Formatted the code

89be4ba

Signed-off-by: 11happy <[email protected]>

llvmbot added the clang-tidy label Jan 11, 2024

small fix

e1b65a9

Signed-off-by: 11happy <[email protected]>

EugeneZelenko reviewed Jan 11, 2024

View reviewed changes

PiotrZSL reviewed Jan 11, 2024

View reviewed changes

felix642 reviewed Jan 11, 2024

View reviewed changes

clang-tools-extra/test/clang-tidy/checkers/readability/ConditionalToStdMinMax.cpp Outdated Show resolved Hide resolved

11happy added 6 commits January 12, 2024 15:43

Revert "small fix"

042ddc3

This reverts commit e1b65a9.

Revert "Formatted the code"

c17b298

This reverts commit 89be4ba.

Revert "Add readability check to suggest replacement of conditional s…

9aad658

…tatement with std::min/std::max" This reverts commit 1883d98.

Added Suggested Changes

2fc0938

Signed-off-by: 11happy <[email protected]>

Formatted the Code

1139ac4

Signed-off-by: 11happy <[email protected]>

Added Release Notes

f73e9f0

Signed-off-by: 11happy <[email protected]>

corrected file name

faf3312

Signed-off-by: 11happy <[email protected]>

11happy requested review from PiotrZSL and EugeneZelenko January 12, 2024 12:02

corrected title length in docs

5ed9896

Signed-off-by: 11happy <[email protected]>

EugeneZelenko reviewed Jan 12, 2024

View reviewed changes

Suggested Changes

19c4b63

Signed-off-by: 11happy <[email protected]>

EugeneZelenko reviewed Jan 12, 2024

View reviewed changes

body indentation

8167787

Signed-off-by: 11happy <[email protected]>

EugeneZelenko reviewed Jan 13, 2024

View reviewed changes

added intialisation,constexpr tests

3a28399

Signed-off-by: 11happy <[email protected]>

EugeneZelenko reviewed Jan 13, 2024

View reviewed changes

clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp Show resolved Hide resolved

clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp Outdated Show resolved Hide resolved

11happy added 3 commits January 28, 2024 00:09

removed whitespace

d8df8c8

Signed-off-by: 11happy <[email protected]>

remove unsed initialisation

8a5d585

Signed-off-by: 11happy <[email protected]>

small change

85fb67d

Signed-off-by: 11happy <[email protected]>

PiotrZSL requested changes Jan 28, 2024

View reviewed changes

PiotrZSL reviewed Jan 28, 2024

View reviewed changes

clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp Outdated Show resolved Hide resolved

simplify

1214547

Signed-off-by: 11happy <[email protected]>

11happy requested a review from PiotrZSL January 29, 2024 00:48

PiotrZSL reviewed Jan 29, 2024

View reviewed changes

small fix

6aef713

Signed-off-by: 11happy <[email protected]>

11happy requested a review from PiotrZSL January 30, 2024 09:35

non template alias

f03b2ff

Signed-off-by: 11happy <[email protected]>

PiotrZSL approved these changes Feb 4, 2024

View reviewed changes

felix642 approved these changes Feb 4, 2024

View reviewed changes

5chmidti approved these changes Feb 5, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp Outdated Show resolved Hide resolved

11happy added 2 commits February 5, 2024 05:46

simplify pointer dereference

5d504a7

Signed-off-by: 11happy <[email protected]>

remove extra parentheses

19cc1f3

Signed-off-by: 11happy <[email protected]>

EugeneZelenko reviewed Feb 5, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.h Show resolved Hide resolved

use spefici type instead of auto

a316744

Signed-off-by: 11happy <[email protected]>

11happy requested a review from EugeneZelenko February 5, 2024 01:56

EugeneZelenko reviewed Feb 5, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp Outdated Show resolved Hide resolved

use auto for cast kind

f0ef998

Signed-off-by: 11happy <[email protected]>

EugeneZelenko approved these changes Feb 5, 2024

View reviewed changes

PiotrZSL merged commit c13e271 into llvm:main Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

11happy commented Jan 11, 2024

llvmbot commented Jan 11, 2024

EugeneZelenko left a comment

PiotrZSL left a comment

11happy commented Jan 11, 2024

5chmidti commented Jan 12, 2024 •

edited

Loading

11happy commented Jan 12, 2024

11happy commented Jan 13, 2024

11happy commented Jan 13, 2024

PiotrZSL left a comment

PiotrZSL left a comment

11happy commented Jan 30, 2024

11happy commented Feb 1, 2024 •

edited

Loading

11happy commented Feb 4, 2024

PiotrZSL left a comment

felix642 left a comment

5chmidti left a comment

PiotrZSL commented Feb 5, 2024

11happy commented Feb 6, 2024

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

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

Conversation

11happy commented Jan 11, 2024

llvmbot commented Jan 11, 2024

EugeneZelenko left a comment

Choose a reason for hiding this comment

PiotrZSL left a comment

Choose a reason for hiding this comment

11happy commented Jan 11, 2024

5chmidti commented Jan 12, 2024 • edited Loading

11happy commented Jan 12, 2024

11happy commented Jan 13, 2024

11happy commented Jan 13, 2024

PiotrZSL left a comment

Choose a reason for hiding this comment

PiotrZSL left a comment

Choose a reason for hiding this comment

11happy commented Jan 30, 2024

11happy commented Feb 1, 2024 • edited Loading

11happy commented Feb 4, 2024

PiotrZSL left a comment

Choose a reason for hiding this comment

felix642 left a comment

Choose a reason for hiding this comment

5chmidti left a comment

Choose a reason for hiding this comment

PiotrZSL commented Feb 5, 2024

11happy commented Feb 6, 2024

5chmidti commented Jan 12, 2024 •

edited

Loading

11happy commented Feb 1, 2024 •

edited

Loading