[clang-tidy] use correct template type in std::min and std::max when operand is integer literal for readability-use-std-min-max

[HerrCai0907]

When comparing with integer literal, integer promote will happen to promote type which has less bit width than int to int or unsigned int.
It will let auto-fix provide correct but out of expected fix.

e.g.

short a;
if ( a > 10 )
  a = 10;

will be

short a;
if ( (int)a > 10 )
  a = (short)10;

which will be fixed as

short a;
a = std::max<int>(a, 10);

but actually it can be

short a;
a = std::max<short>(a, 10);

Fixed: #121676

[HerrCai0907]

• [clang-tidy] use correct template type in std::min and std::max when operand is integer literal for readability-use-std-min-max #122296 👈 (View in Graphite)
• [clang-tidy][NFC] clean readability-use-std-min-max #122288
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Congcong Cai (HerrCai0907)

Changes

When comparing with integer literal, integer promote will happen to promote type which has less bit width than int to int or unsigned int.
It will let auto-fix provide correct but out of expected fix.

e.g.

short a;
if ( a > 10 )
  a = 10;

will be

short a;
if ( (int)a > 10 )
  a = (short)10;

which will be fixed as

short a;
a = std::max<int>(a, 10);

but actually it can be

short a;
a = std::max<short>(a, 10);

---

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

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp (+23-12)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp (+21)

diff --git a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp
index 179173502a8d01..6f6b8a853a91e0 100644
--- a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp
@@ -79,6 +79,27 @@ static QualType getNonTemplateAlias(QualType QT) {
   return QT;
 }
 
+static QualType getReplacementCastType(const Expr *CondLhs, const Expr *CondRhs,
+                                       QualType ComparedType) {
+  QualType LhsType = CondLhs->getType();
+  QualType RhsType = CondRhs->getType();
+  QualType LhsCanonicalType =
+      LhsType.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+  QualType RhsCanonicalType =
+      RhsType.getCanonicalType().getNonReferenceType().getUnqualifiedType();
+  QualType GlobalImplicitCastType;
+  if (LhsCanonicalType != RhsCanonicalType) {
+    if (llvm::isa<IntegerLiteral>(CondRhs)) {
+      GlobalImplicitCastType = getNonTemplateAlias(LhsType);
+    } else if (llvm::isa<IntegerLiteral>(CondLhs)) {
+      GlobalImplicitCastType = getNonTemplateAlias(RhsType);
+    } else {
+      GlobalImplicitCastType = getNonTemplateAlias(ComparedType);
+    }
+  }
+  return GlobalImplicitCastType;
+}
+
 static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs,
                                      const Expr *AssignLhs,
                                      const SourceManager &Source,
@@ -92,18 +113,8 @@ static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs,
   const llvm::StringRef AssignLhsStr = Lexer::getSourceText(
       Source.getExpansionRange(AssignLhs->getSourceRange()), Source, LO);
 
-  QualType GlobalImplicitCastType;
-  QualType LhsType = CondLhs->getType()
-                         .getCanonicalType()
-                         .getNonReferenceType()
-                         .getUnqualifiedType();
-  QualType RhsType = CondRhs->getType()
-                         .getCanonicalType()
-                         .getNonReferenceType()
-                         .getUnqualifiedType();
-  if (LhsType != RhsType) {
-    GlobalImplicitCastType = getNonTemplateAlias(BO->getLHS()->getType());
-  }
+  QualType GlobalImplicitCastType =
+      getReplacementCastType(CondLhs, CondRhs, BO->getLHS()->getType());
 
   return (AssignLhsStr + " = " + FunctionName +
           (!GlobalImplicitCastType.isNull()
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 94e15639c4a92e..fd523da8dc5a1b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -364,6 +364,10 @@ Changes in existing checks
   <clang-tidy/checks/readability/identifier-naming>` check to
   validate ``namespace`` aliases.
 
+- Improved :doc:`readability-use-std-min-max
+  <clang-tidy/checks/readability/use-std-min-max>` check to use correct template
+  type in ``std::min`` and ``std::max`` when operand is integer literal.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
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
index 9c0e2eabda348d..35ade8a7c6d37e 100644
--- 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
@@ -252,3 +252,24 @@ void testVectorSizeType() {
   if (value < v.size())
     value = v.size();
 }
+
+namespace gh121676 {
+
+void useLeft() {
+  using U16 = unsigned short;
+  U16 I = 0;
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max]
+  // CHECK-FIXES: I = std::max<U16>(I, 16U);
+  if (I < 16U)
+    I = 16U;
+}
+void useRight() {
+  using U16 = unsigned short;
+  U16 I = 0;
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max]
+  // CHECK-FIXES: I = std::min<U16>(16U, I);
+  if (16U < I)
+    I = 16U;
+}
+
+} // namespace gh121676

[PiotrZSL]

LGTM.