[clang-tidy] fix false positive in modernize-min-max-use-initializer-list

[5chmidti]

Previously, whenever a replacement was generated by the analysis, a
diagnostic was generated. This became an issue when a call to
std::min or std::max consisted only of an initializer list with at
least one argument to the list requiring a type cast.
In this case, a single replacement that added a static_cast was created,
that resulted in a diagnostic being issued but with no nested call
to std::min or std::max.

Instead, explicitly track if a nested call was detected and only emit a
diagnostic if this is the case.

Fixes #107594

[llvmbot]

@llvm/pr-subscribers-clang-tidy

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

Author: Julian Schmidt (5chmidti)

Changes

Previously, whenever a replacement was generated by the analysis, a
diagnostic was generated. This became an issue when a call to
std::min or std::max consisted only of an initializer list with at
least one argument to the list requiring a type cast.
In this case, a single replacement was created that resulted in an
issued diagnostic.

Instead, explicitly track if a nested call was detected and only emit a
diagnostic if this is the case.

Fixes #107594

---

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

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp (+17-13)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
• (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp (+6)

diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
index 418699ffbc4d1a..f4afae2fe6e79e 100644
--- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
@@ -72,11 +72,10 @@ static FindArgsResult findArgs(const CallExpr *Call) {
   return Result;
 }
 
-static SmallVector<FixItHint>
-generateReplacements(const MatchFinder::MatchResult &Match,
-                     const CallExpr *TopCall, const FindArgsResult &Result,
-                     const bool IgnoreNonTrivialTypes,
-                     const std::uint64_t IgnoreTrivialTypesOfSizeAbove) {
+static std::pair<bool, SmallVector<FixItHint>> generateReplacements(
+    const MatchFinder::MatchResult &Match, const CallExpr *TopCall,
+    const FindArgsResult &Result, const bool IgnoreNonTrivialTypes,
+    const std::uint64_t IgnoreTrivialTypesOfSizeAbove, bool FoundNestedCall) {
   SmallVector<FixItHint> FixItHints;
   const SourceManager &SourceMngr = *Match.SourceManager;
   const LangOptions &LanguageOpts = Match.Context->getLangOpts();
@@ -91,13 +90,13 @@ generateReplacements(const MatchFinder::MatchResult &Match,
   const bool IsResultTypeTrivial = ResultType.isTrivialType(*Match.Context);
 
   if ((!IsResultTypeTrivial && IgnoreNonTrivialTypes))
-    return FixItHints;
+    return {false, FixItHints};
 
   if (IsResultTypeTrivial &&
       static_cast<std::uint64_t>(
           Match.Context->getTypeSizeInChars(ResultType).getQuantity()) >
           IgnoreTrivialTypesOfSizeAbove)
-    return FixItHints;
+    return {false, FixItHints};
 
   for (const Expr *Arg : Result.Args) {
     const auto *InnerCall = dyn_cast<CallExpr>(Arg->IgnoreParenImpCasts());
@@ -146,6 +145,9 @@ generateReplacements(const MatchFinder::MatchResult &Match,
                                        *Match.Context))
       continue;
 
+    // We have found a nested call
+    FoundNestedCall = true;
+
     // remove the function call
     FixItHints.push_back(
         FixItHint::CreateRemoval(InnerCall->getCallee()->getSourceRange()));
@@ -168,9 +170,11 @@ generateReplacements(const MatchFinder::MatchResult &Match,
           CharSourceRange::getTokenRange(InnerResult.First->getEndLoc())));
     }
 
-    const SmallVector<FixItHint> InnerReplacements = generateReplacements(
+    const auto [FoundNestedCallInner, InnerReplacements] = generateReplacements(
         Match, InnerCall, InnerResult, IgnoreNonTrivialTypes,
-        IgnoreTrivialTypesOfSizeAbove);
+        IgnoreTrivialTypesOfSizeAbove, false);
+
+    FoundNestedCall |= FoundNestedCallInner;
 
     FixItHints.append(InnerReplacements);
 
@@ -189,7 +193,7 @@ generateReplacements(const MatchFinder::MatchResult &Match,
     }
   }
 
-  return FixItHints;
+  return {FoundNestedCall, FixItHints};
 }
 
 MinMaxUseInitializerListCheck::MinMaxUseInitializerListCheck(
@@ -238,11 +242,11 @@ void MinMaxUseInitializerListCheck::check(
   const auto *TopCall = Match.Nodes.getNodeAs<CallExpr>("topCall");
 
   const FindArgsResult Result = findArgs(TopCall);
-  const SmallVector<FixItHint> Replacements =
+  const auto [FoundNestedCall, Replacements] =
       generateReplacements(Match, TopCall, Result, IgnoreNonTrivialTypes,
-                           IgnoreTrivialTypesOfSizeAbove);
+                           IgnoreTrivialTypesOfSizeAbove, false);
 
-  if (Replacements.empty())
+  if (!FoundNestedCall)
     return;
 
   const DiagnosticBuilder Diagnostic =
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8d028f8863cb7a..dcfe11e58f4710 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -116,6 +116,11 @@ Changes in existing checks
   <clang-tidy/checks/misc/unconventional-assign-operator>` check to avoid
   false positive for C++23 deducing this.
 
+- Improved :doc:`modernize-min-max-use-initializer-list
+  <clang-tidy/checks/modernize/min-max-use-initializer-list>` check by removing
+  a false positive when only an implicit conversion happened inside an
+  initializer list.
+
 - Improved :doc:`modernize-use-std-print
   <clang-tidy/checks/modernize/use-std-print>` check to support replacing
   member function calls too.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp
index c7632fe007a4f4..f4e21316718045 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp
@@ -323,5 +323,11 @@ struct GH91982 {
   }
 };
 
+struct GH107594 {
+    int foo(int a, int b, char c) {
+        return std::max<int>({a, b, c});
+    }
+};
+
 } // namespace
 

[SimplyDanny]

Isn't the result always true with the value being reset to true in line 149 and having an |= here?

[SimplyDanny]

All callers use false as the last argument. Is the new argument needed at all?

[5chmidti]

You're right with both of your comments. I've changed the implementation in the latest commit.

[HerrCai0907]

Could you add some comments of this result. since bool means nothing.

[5chmidti]

done

[HerrCai0907]

LGTM