[clang-tools-extra] 605a9ad - [clang-tidy] fix false positive in modernize-min-max-use-initializer-list (#107649)

[via cfe-commits]

Author: Julian Schmidt
Date: 2024-09-17T10:43:40+02:00
New Revision: 605a9adb4340b347f480a95a6eef3c9045e8416f

URL: https://github.com/llvm/llvm-project/commit/605a9adb4340b347f480a95a6eef3c9045e8416f
DIFF: https://github.com/llvm/llvm-project/commit/605a9adb4340b347f480a95a6eef3c9045e8416f.diff

LOG: [clang-tidy] fix false positive in modernize-min-max-use-initializer-list (#107649)

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

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
    clang-tools-extra/docs/ReleaseNotes.rst
    clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
index 418699ffbc4d1a..9861f4681db1b4 100644
--- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
@@ -72,7 +72,11 @@ static FindArgsResult findArgs(const CallExpr *Call) {
   return Result;
 }
 
-static SmallVector<FixItHint>
+// Returns `true` as `first` only if a nested call to `std::min` or
+// `std::max` was found. Checking if `FixItHint`s were generated is not enough,
+// as the explicit casts that the check introduces may be generated without a
+// nested `std::min` or `std::max` call.
+static std::pair<bool, SmallVector<FixItHint>>
 generateReplacements(const MatchFinder::MatchResult &Match,
                      const CallExpr *TopCall, const FindArgsResult &Result,
                      const bool IgnoreNonTrivialTypes,
@@ -91,13 +95,15 @@ 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};
+
+  bool FoundNestedCall = false;
 
   for (const Expr *Arg : Result.Args) {
     const auto *InnerCall = dyn_cast<CallExpr>(Arg->IgnoreParenImpCasts());
@@ -146,6 +152,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,7 +177,7 @@ generateReplacements(const MatchFinder::MatchResult &Match,
           CharSourceRange::getTokenRange(InnerResult.First->getEndLoc())));
     }
 
-    const SmallVector<FixItHint> InnerReplacements = generateReplacements(
+    const auto [_, InnerReplacements] = generateReplacements(
         Match, InnerCall, InnerResult, IgnoreNonTrivialTypes,
         IgnoreTrivialTypesOfSizeAbove);
 
@@ -189,7 +198,7 @@ generateReplacements(const MatchFinder::MatchResult &Match,
     }
   }
 
-  return FixItHints;
+  return {FoundNestedCall, FixItHints};
 }
 
 MinMaxUseInitializerListCheck::MinMaxUseInitializerListCheck(
@@ -238,11 +247,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);
 
-  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 c297ed88ece0a2..a72c22187d5b4c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,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 fixing
+  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