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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
