[clang-tools-extra] [clang-tidy] fix false positive in modernize-min-max-use-initializer-list (PR #107649)
Julian Schmidt via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 16 14:17:47 PDT 2024
https://github.com/5chmidti updated https://github.com/llvm/llvm-project/pull/107649
>From 097a679f33bdded29fa67833b7fcd4707057cbf3 Mon Sep 17 00:00:00 2001
From: Julian Schmidt <git.julian.schmidt at gmail.com>
Date: Sat, 7 Sep 2024 00:10:08 +0200
Subject: [PATCH 1/4] [clang-tidy] fix false positive in
modernize-min-max-use-initializer-list
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
---
.../MinMaxUseInitializerListCheck.cpp | 30 +++++++++++--------
clang-tools-extra/docs/ReleaseNotes.rst | 5 ++++
.../min-max-use-initializer-list.cpp | 6 ++++
3 files changed, 28 insertions(+), 13 deletions(-)
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
>From 7b0b56a45a3ef699ed3a8fb5d55bbf1dd2e602a9 Mon Sep 17 00:00:00 2001
From: Julian Schmidt <git.julian.schmidt at gmail.com>
Date: Wed, 11 Sep 2024 17:52:17 +0200
Subject: [PATCH 2/4] fix impl
---
.../MinMaxUseInitializerListCheck.cpp | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
index f4afae2fe6e79e..896ab14dbed536 100644
--- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
@@ -72,10 +72,11 @@ static FindArgsResult findArgs(const CallExpr *Call) {
return Result;
}
-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) {
+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) {
SmallVector<FixItHint> FixItHints;
const SourceManager &SourceMngr = *Match.SourceManager;
const LangOptions &LanguageOpts = Match.Context->getLangOpts();
@@ -98,6 +99,8 @@ static std::pair<bool, SmallVector<FixItHint>> generateReplacements(
IgnoreTrivialTypesOfSizeAbove)
return {false, FixItHints};
+ bool FoundNestedCall = false;
+
for (const Expr *Arg : Result.Args) {
const auto *InnerCall = dyn_cast<CallExpr>(Arg->IgnoreParenImpCasts());
@@ -170,11 +173,9 @@ static std::pair<bool, SmallVector<FixItHint>> generateReplacements(
CharSourceRange::getTokenRange(InnerResult.First->getEndLoc())));
}
- const auto [FoundNestedCallInner, InnerReplacements] = generateReplacements(
+ const auto [_, InnerReplacements] = generateReplacements(
Match, InnerCall, InnerResult, IgnoreNonTrivialTypes,
- IgnoreTrivialTypesOfSizeAbove, false);
-
- FoundNestedCall |= FoundNestedCallInner;
+ IgnoreTrivialTypesOfSizeAbove);
FixItHints.append(InnerReplacements);
@@ -244,7 +245,7 @@ void MinMaxUseInitializerListCheck::check(
const FindArgsResult Result = findArgs(TopCall);
const auto [FoundNestedCall, Replacements] =
generateReplacements(Match, TopCall, Result, IgnoreNonTrivialTypes,
- IgnoreTrivialTypesOfSizeAbove, false);
+ IgnoreTrivialTypesOfSizeAbove);
if (!FoundNestedCall)
return;
>From 4bfc2688909cdd13ddf0bd260503d375f4d3284c Mon Sep 17 00:00:00 2001
From: Julian Schmidt <git.julian.schmidt at gmail.com>
Date: Mon, 16 Sep 2024 23:12:30 +0200
Subject: [PATCH 3/4] address review comment
---
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index dcfe11e58f4710..bc8df0a9a424c7 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -117,7 +117,7 @@ Changes in existing checks
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
+ <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.
>From 6b611dc9a58519df6cefa8084ea74410f3f87ed3 Mon Sep 17 00:00:00 2001
From: Julian Schmidt <git.julian.schmidt at gmail.com>
Date: Mon, 16 Sep 2024 23:17:24 +0200
Subject: [PATCH 4/4] add comment explaining the `pair` structure
---
.../clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
index 896ab14dbed536..9861f4681db1b4 100644
--- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp
@@ -72,6 +72,10 @@ static FindArgsResult findArgs(const CallExpr *Call) {
return Result;
}
+// 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,
More information about the cfe-commits
mailing list