https://github.com/sopyb updated https://github.com/llvm/llvm-project/pull/85572
>From 17d6ad65216237f9a325d9b608c1372cbbf83ff0 Mon Sep 17 00:00:00 2001 From: sopy <cont...@sopy.one> Date: Sun, 17 Mar 2024 17:30:27 +0200 Subject: [PATCH 1/6] [clang-tidy] add check to suggest replacement of nested std::min or std::max with initializer lists --- .../clang-tidy/modernize/CMakeLists.txt | 1 + .../MinMaxUseInitializerListCheck.cpp | 138 ++++++++++++++++++ .../modernize/MinMaxUseInitializerListCheck.h | 58 ++++++++ .../modernize/ModernizeTidyModule.cpp | 3 + clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../docs/clang-tidy/checks/list.rst | 1 + 6 files changed, 207 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index 6852db6c2ee311..8005d6e91c060c 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -16,6 +16,7 @@ add_clang_library(clangTidyModernizeModule MakeSharedCheck.cpp MakeSmartPtrCheck.cpp MakeUniqueCheck.cpp + MinMaxUseInitializerListCheck.cpp ModernizeTidyModule.cpp PassByValueCheck.cpp RawStringLiteralCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp new file mode 100644 index 00000000000000..b7dc3ff436f6e3 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp @@ -0,0 +1,138 @@ +//===--- MinMaxUseInitializerListCheck.cpp - clang-tidy -------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "MinMaxUseInitializerListCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::modernize { + +MinMaxUseInitializerListCheck::MinMaxUseInitializerListCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + Inserter(Options.getLocalOrGlobal("IncludeStyle", + utils::IncludeSorter::IS_LLVM), + areDiagsSelfContained()) {} + +void MinMaxUseInitializerListCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", Inserter.getStyle()); +} + +void MinMaxUseInitializerListCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr( + callee(functionDecl(hasName("::std::max"))), + hasAnyArgument(callExpr(callee(functionDecl(hasName("::std::max"))))), + unless( + hasParent(callExpr(callee(functionDecl(hasName("::std::max"))))))) + .bind("maxCall"), + this); + + Finder->addMatcher( + callExpr( + callee(functionDecl(hasName("::std::min"))), + hasAnyArgument(callExpr(callee(functionDecl(hasName("::std::min"))))), + unless( + hasParent(callExpr(callee(functionDecl(hasName("::std::min"))))))) + .bind("minCall"), + this); +} + +void MinMaxUseInitializerListCheck::registerPPCallbacks( + const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) { + Inserter.registerPreprocessor(PP); +} + +void MinMaxUseInitializerListCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MaxCall = Result.Nodes.getNodeAs<CallExpr>("maxCall"); + const auto *MinCall = Result.Nodes.getNodeAs<CallExpr>("minCall"); + + const CallExpr *TopCall = MaxCall ? MaxCall : MinCall; + if (!TopCall) { + return; + } + const QualType ResultType = + TopCall->getDirectCallee()->getReturnType().getNonReferenceType(); + + const Expr *FirstArg = nullptr; + const Expr *LastArg = nullptr; + std::vector<const Expr *> Args; + findArgs(TopCall, &FirstArg, &LastArg, Args); + + if (!FirstArg || !LastArg || Args.size() <= 2) { + return; + } + + std::string ReplacementText = "{"; + for (const Expr *Arg : Args) { + QualType ArgType = Arg->getType(); + bool CastNeeded = + ArgType.getCanonicalType() != ResultType.getCanonicalType(); + + if (CastNeeded) + ReplacementText += "static_cast<" + ResultType.getAsString() + ">("; + + ReplacementText += Lexer::getSourceText( + CharSourceRange::getTokenRange(Arg->getSourceRange()), + *Result.SourceManager, Result.Context->getLangOpts()); + + if (CastNeeded) + ReplacementText += ")"; + ReplacementText += ", "; + } + ReplacementText = ReplacementText.substr(0, ReplacementText.size() - 2) + "}"; + + diag(TopCall->getBeginLoc(), + "do not use nested std::%0 calls, use %1 instead") + << TopCall->getDirectCallee()->getName() << ReplacementText + << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange( + FirstArg->getBeginLoc(), + Lexer::getLocForEndOfToken(TopCall->getEndLoc(), 0, + Result.Context->getSourceManager(), + Result.Context->getLangOpts()) + .getLocWithOffset(-2)), + ReplacementText) + << Inserter.createMainFileIncludeInsertion("<algorithm>"); +} + +void MinMaxUseInitializerListCheck::findArgs(const CallExpr *Call, + const Expr **First, + const Expr **Last, + std::vector<const Expr *> &Args) { + if (!Call) { + return; + } + + const FunctionDecl *Callee = Call->getDirectCallee(); + if (!Callee) { + return; + } + + for (const Expr *Arg : Call->arguments()) { + if (!*First) + *First = Arg; + + const CallExpr *InnerCall = dyn_cast<CallExpr>(Arg); + if (InnerCall && InnerCall->getDirectCallee() && + InnerCall->getDirectCallee()->getNameAsString() == + Call->getDirectCallee()->getNameAsString()) { + findArgs(InnerCall, First, Last, Args); + } else + Args.push_back(Arg); + + *Last = Arg; + } +} + +} // namespace clang::tidy::modernize diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h new file mode 100644 index 00000000000000..dc111d4ce7800e --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h @@ -0,0 +1,58 @@ +//===--- MinMaxUseInitializerListCheck.h - clang-tidy -----------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MINMAXUSEINITIALIZERLISTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MINMAXUSEINITIALIZERLISTCHECK_H + +#include "../ClangTidyCheck.h" +#include "../utils/IncludeInserter.h" + +namespace clang::tidy::modernize { + +/// Transforms the repeated calls to `std::min` and `std::max` into a single +/// call using initializer lists. +/// +/// It identifies cases where `std::min` or `std::max` is used to find the +/// minimum or maximum value among more than two items through repeated calls. +/// The check replaces these calls with a single call to `std::min` or +/// `std::max` that uses an initializer list. This makes the code slightly more +/// efficient. +/// \n\n +/// For example: +/// +/// \code +/// int a = std::max(std::max(i, j), k); +/// \endcode +/// +/// This code is transformed to: +/// +/// \code +/// int a = std::max({i, j, k}); +/// \endcode +class MinMaxUseInitializerListCheck : public ClangTidyCheck { +public: + MinMaxUseInitializerListCheck(StringRef Name, ClangTidyContext *Context); + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + utils::IncludeInserter Inserter; + void findArgs(const CallExpr *call, const Expr **first, const Expr **last, + std::vector<const Expr *> &args); +}; + +} // namespace clang::tidy::modernize + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MINMAXUSEINITIALIZERLISTCHECK_H diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index e96cf274f58cfe..776558433c5baa 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -18,6 +18,7 @@ #include "MacroToEnumCheck.h" #include "MakeSharedCheck.h" #include "MakeUniqueCheck.h" +#include "MinMaxUseInitializerListCheck.h" #include "PassByValueCheck.h" #include "RawStringLiteralCheck.h" #include "RedundantVoidArgCheck.h" @@ -68,6 +69,8 @@ class ModernizeModule : public ClangTidyModule { CheckFactories.registerCheck<MacroToEnumCheck>("modernize-macro-to-enum"); CheckFactories.registerCheck<MakeSharedCheck>("modernize-make-shared"); CheckFactories.registerCheck<MakeUniqueCheck>("modernize-make-unique"); + CheckFactories.registerCheck<MinMaxUseInitializerListCheck>( + "modernize-min-max-use-initializer-list"); CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value"); CheckFactories.registerCheck<UseDesignatedInitializersCheck>( "modernize-use-designated-initializers"); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index a604e9276668ae..e743438f589987 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -117,6 +117,12 @@ New checks to reading out-of-bounds data due to inadequate or incorrect string null termination. +- New :doc:`modernize-min-max-use-initializer-list + <clang-tidy/checks/modernize/min-max-use-initializer-list>` check. + + Replaces chained ``std::min`` and ``std::max`` calls that can be written as + initializer lists. + - New :doc:`modernize-use-designated-initializers <clang-tidy/checks/modernize/use-designated-initializers>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 79e81dd174e4f3..4809d704a94646 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -275,6 +275,7 @@ Clang-Tidy Checks :doc:`modernize-macro-to-enum <modernize/macro-to-enum>`, "Yes" :doc:`modernize-make-shared <modernize/make-shared>`, "Yes" :doc:`modernize-make-unique <modernize/make-unique>`, "Yes" + :doc:`modernize-min-max-use-initializer-list <modernize/min-max-use-initializer-list>`, "Yes" :doc:`modernize-pass-by-value <modernize/pass-by-value>`, "Yes" :doc:`modernize-raw-string-literal <modernize/raw-string-literal>`, "Yes" :doc:`modernize-redundant-void-arg <modernize/redundant-void-arg>`, "Yes" >From 1bed756e8d45c48be834bd0d12610bb05ede478e Mon Sep 17 00:00:00 2001 From: sopy <cont...@sopy.one> Date: Wed, 20 Mar 2024 12:11:34 +0200 Subject: [PATCH 2/6] Summary: Refactor min-max-use-initializer-list.cpp and add tests Details: - Refactored min-max-use-initializer-list.cpp to handle cases where std::{min,max} is given a compare function argument - Shortened documentation in MinMaxUseInitializerListCheck.h - Added tests for min-modernize-min-max-use-initializer-list - Added documentation for min-modernize-min-max-use-initializer-list - Updated ReleaseNotes.rst based on mantainer suggestions --- .../MinMaxUseInitializerListCheck.cpp | 176 ++++++++++++------ .../modernize/MinMaxUseInitializerListCheck.h | 35 ++-- clang-tools-extra/docs/ReleaseNotes.rst | 4 +- .../min-max-use-initializer-list.rst | 26 +++ .../min-max-use-initializer-list.cpp | 105 +++++++++++ 5 files changed, 270 insertions(+), 76 deletions(-) create mode 100644 clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp index b7dc3ff436f6e3..ddeac5b1b3ac77 100644 --- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp @@ -31,19 +31,25 @@ void MinMaxUseInitializerListCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( callExpr( callee(functionDecl(hasName("::std::max"))), - hasAnyArgument(callExpr(callee(functionDecl(hasName("::std::max"))))), + anyOf(hasArgument( + 0, callExpr(callee(functionDecl(hasName("::std::max"))))), + hasArgument( + 1, callExpr(callee(functionDecl(hasName("::std::max")))))), unless( hasParent(callExpr(callee(functionDecl(hasName("::std::max"))))))) - .bind("maxCall"), + .bind("topCall"), this); Finder->addMatcher( callExpr( callee(functionDecl(hasName("::std::min"))), - hasAnyArgument(callExpr(callee(functionDecl(hasName("::std::min"))))), + anyOf(hasArgument( + 0, callExpr(callee(functionDecl(hasName("::std::min"))))), + hasArgument( + 1, callExpr(callee(functionDecl(hasName("::std::min")))))), unless( hasParent(callExpr(callee(functionDecl(hasName("::std::min"))))))) - .bind("minCall"), + .bind("topCall"), this); } @@ -53,29 +59,112 @@ void MinMaxUseInitializerListCheck::registerPPCallbacks( } void MinMaxUseInitializerListCheck::check( - const MatchFinder::MatchResult &Result) { - const auto *MaxCall = Result.Nodes.getNodeAs<CallExpr>("maxCall"); - const auto *MinCall = Result.Nodes.getNodeAs<CallExpr>("minCall"); + const MatchFinder::MatchResult &Match) { + const CallExpr *TopCall = Match.Nodes.getNodeAs<CallExpr>("topCall"); + MinMaxUseInitializerListCheck::FindArgsResult Result = + findArgs(Match, TopCall); - const CallExpr *TopCall = MaxCall ? MaxCall : MinCall; - if (!TopCall) { + if (!Result.First || !Result.Last || Result.Args.size() <= 2) { return; } - const QualType ResultType = - TopCall->getDirectCallee()->getReturnType().getNonReferenceType(); - const Expr *FirstArg = nullptr; - const Expr *LastArg = nullptr; - std::vector<const Expr *> Args; - findArgs(TopCall, &FirstArg, &LastArg, Args); + std::string ReplacementText = generateReplacement(Match, TopCall, Result); - if (!FirstArg || !LastArg || Args.size() <= 2) { - return; + diag(TopCall->getBeginLoc(), + "do not use nested std::%0 calls, use %1 instead") + << TopCall->getDirectCallee()->getName() << ReplacementText + << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(TopCall->getBeginLoc(), + TopCall->getEndLoc()), + ReplacementText) + << Inserter.createMainFileIncludeInsertion("<algorithm>"); +} + +MinMaxUseInitializerListCheck::FindArgsResult +MinMaxUseInitializerListCheck::findArgs(const MatchFinder::MatchResult &Match, + const CallExpr *Call) { + FindArgsResult Result; + Result.First = nullptr; + Result.Last = nullptr; + Result.Compare = nullptr; + + if (Call->getNumArgs() > 2) { + auto argIterator = Call->arguments().begin(); + std::advance(argIterator, 2); + Result.Compare = *argIterator; } - std::string ReplacementText = "{"; - for (const Expr *Arg : Args) { + for (const Expr *Arg : Call->arguments()) { + if (!Result.First) + Result.First = Arg; + + const CallExpr *InnerCall = dyn_cast<CallExpr>(Arg); + if (InnerCall && InnerCall->getDirectCallee() && + InnerCall->getDirectCallee()->getNameAsString() == + Call->getDirectCallee()->getNameAsString()) { + FindArgsResult InnerResult = findArgs(Match, InnerCall); + + bool processInnerResult = false; + + if (!Result.Compare && !InnerResult.Compare) + processInnerResult = true; + else if (Result.Compare && InnerResult.Compare && + Lexer::getSourceText(CharSourceRange::getTokenRange( + Result.Compare->getSourceRange()), + *Match.SourceManager, + Match.Context->getLangOpts()) == + Lexer::getSourceText( + CharSourceRange::getTokenRange( + InnerResult.Compare->getSourceRange()), + *Match.SourceManager, Match.Context->getLangOpts())) + processInnerResult = true; + + if (processInnerResult) { + Result.Args.insert(Result.Args.end(), InnerResult.Args.begin(), + InnerResult.Args.end()); + continue; + } + } + + if (Arg == Result.Compare) + continue; + + Result.Args.push_back(Arg); + Result.Last = Arg; + } + + return Result; +} + +std::string MinMaxUseInitializerListCheck::generateReplacement( + const MatchFinder::MatchResult &Match, const CallExpr *TopCall, + const FindArgsResult Result) { + std::string ReplacementText = + Lexer::getSourceText( + CharSourceRange::getTokenRange( + TopCall->getBeginLoc(), + Result.First->getBeginLoc().getLocWithOffset(-1)), + *Match.SourceManager, Match.Context->getLangOpts()) + .str() + + "{"; + const QualType ResultType = + TopCall->getDirectCallee()->getReturnType().getNonReferenceType(); + + for (const Expr *Arg : Result.Args) { QualType ArgType = Arg->getType(); + + // check if expression is std::min or std::max + if (const auto *InnerCall = dyn_cast<CallExpr>(Arg)) { + if (InnerCall->getDirectCallee() && + InnerCall->getDirectCallee()->getNameAsString() != + TopCall->getDirectCallee()->getNameAsString()) { + FindArgsResult innerResult = findArgs(Match, InnerCall); + ReplacementText += generateReplacement(Match, InnerCall, innerResult) += + "})"; + continue; + } + } + bool CastNeeded = ArgType.getCanonicalType() != ResultType.getCanonicalType(); @@ -84,55 +173,22 @@ void MinMaxUseInitializerListCheck::check( ReplacementText += Lexer::getSourceText( CharSourceRange::getTokenRange(Arg->getSourceRange()), - *Result.SourceManager, Result.Context->getLangOpts()); + *Match.SourceManager, Match.Context->getLangOpts()); if (CastNeeded) ReplacementText += ")"; ReplacementText += ", "; } ReplacementText = ReplacementText.substr(0, ReplacementText.size() - 2) + "}"; - - diag(TopCall->getBeginLoc(), - "do not use nested std::%0 calls, use %1 instead") - << TopCall->getDirectCallee()->getName() << ReplacementText - << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange( - FirstArg->getBeginLoc(), - Lexer::getLocForEndOfToken(TopCall->getEndLoc(), 0, - Result.Context->getSourceManager(), - Result.Context->getLangOpts()) - .getLocWithOffset(-2)), - ReplacementText) - << Inserter.createMainFileIncludeInsertion("<algorithm>"); -} - -void MinMaxUseInitializerListCheck::findArgs(const CallExpr *Call, - const Expr **First, - const Expr **Last, - std::vector<const Expr *> &Args) { - if (!Call) { - return; - } - - const FunctionDecl *Callee = Call->getDirectCallee(); - if (!Callee) { - return; + if (Result.Compare) { + ReplacementText += ", "; + ReplacementText += Lexer::getSourceText( + CharSourceRange::getTokenRange(Result.Compare->getSourceRange()), + *Match.SourceManager, Match.Context->getLangOpts()); } + ReplacementText += ")"; - for (const Expr *Arg : Call->arguments()) { - if (!*First) - *First = Arg; - - const CallExpr *InnerCall = dyn_cast<CallExpr>(Arg); - if (InnerCall && InnerCall->getDirectCallee() && - InnerCall->getDirectCallee()->getNameAsString() == - Call->getDirectCallee()->getNameAsString()) { - findArgs(InnerCall, First, Last, Args); - } else - Args.push_back(Arg); - - *Last = Arg; - } + return ReplacementText; } } // namespace clang::tidy::modernize diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h index dc111d4ce7800e..43d95004511430 100644 --- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h @@ -14,15 +14,9 @@ namespace clang::tidy::modernize { -/// Transforms the repeated calls to `std::min` and `std::max` into a single -/// call using initializer lists. +/// Replaces chained ``std::min`` and ``std::max`` calls with a initializer list +/// where applicable. /// -/// It identifies cases where `std::min` or `std::max` is used to find the -/// minimum or maximum value among more than two items through repeated calls. -/// The check replaces these calls with a single call to `std::min` or -/// `std::max` that uses an initializer list. This makes the code slightly more -/// efficient. -/// \n\n /// For example: /// /// \code @@ -38,19 +32,32 @@ class MinMaxUseInitializerListCheck : public ClangTidyCheck { public: MinMaxUseInitializerListCheck(StringRef Name, ClangTidyContext *Context); - bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { - return LangOpts.CPlusPlus11; - } void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) override; - void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void check(const ast_matchers::MatchFinder::MatchResult &Match) override; + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } private: + struct FindArgsResult { + const Expr *First; + const Expr *Last; + const Expr *Compare; + std::vector<const Expr *> Args; + }; utils::IncludeInserter Inserter; - void findArgs(const CallExpr *call, const Expr **first, const Expr **last, - std::vector<const Expr *> &args); + FindArgsResult findArgs(const ast_matchers::MatchFinder::MatchResult &Match, + const CallExpr *Call); + std::string + generateReplacement(const ast_matchers::MatchFinder::MatchResult &Match, + const CallExpr *TopCall, const FindArgsResult Result); }; } // namespace clang::tidy::modernize diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e743438f589987..13d14fa9ccf0aa 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -120,8 +120,8 @@ New checks - New :doc:`modernize-min-max-use-initializer-list <clang-tidy/checks/modernize/min-max-use-initializer-list>` check. - Replaces chained ``std::min`` and ``std::max`` calls that can be written as - initializer lists. + Replaces chained ``std::min`` and ``std::max`` calls with a initializer list + where applicable. - New :doc:`modernize-use-designated-initializers <clang-tidy/checks/modernize/use-designated-initializers>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst new file mode 100644 index 00000000000000..0748e180897948 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst @@ -0,0 +1,26 @@ +.. title:: clang-tidy - modernize-min-max-use-initializer-list + +modernize-min-max-use-initializer-list +====================================== + +Replaces chained ``std::min`` and ``std::max`` calls with a initializer list where applicable. + +For instance, consider the following code: + +.. code-block:: cpp + + int a = std::max(std::max(i, j), k); + +`modernize-min-max-use-initializer-list` check will transform the above code to: + +.. code-block:: cpp + + int a = std::max({i, j, k}); + +Options +======= + +.. option:: IncludeStyle + + A string specifying which include-style is used, `llvm` or `google`. Default + is `llvm`. \ No newline at end of file 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 new file mode 100644 index 00000000000000..3e15774f7b3f14 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp @@ -0,0 +1,105 @@ +// RUN: %check_clang_tidy %s modernize-min-max-use-initializer-list %t + +// CHECK-FIXES: #include <algorithm> +namespace std { +template< class T > +const T& max( const T& a, const T& b ) { + return (a < b) ? b : a; +}; + +template< class T, class Compare > +const T& max( const T& a, const T& b, Compare comp ) { + return (comp(a, b)) ? b : a; +}; + +template< class T > +const T& min( const T& a, const T& b ) { + return (b < a) ? b : a; +}; + +template< class T, class Compare > +const T& min( const T& a, const T& b, Compare comp ) { + return (comp(b, a)) ? b : a; +}; +} + +using namespace std; + +namespace { +bool fless_than(int a, int b) { +return a < b; +} + +bool fgreater_than(int a, int b) { +return a > b; +} +auto less_than = [](int a, int b) { return a < b; }; +auto greater_than = [](int a, int b) { return a > b; }; + +int max1 = std::max(1, std::max(2, 3)); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({1, 2, 3}) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max1 = std::max({1, 2, 3}); + +int min1 = std::min(1, std::min(2, 3)); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({1, 2, 3}) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int min1 = std::min({1, 2, 3}); + +int max2 = std::max(1, std::max(2, std::max(3, 4))); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({1, 2, 3, 4}) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max2 = std::max({1, 2, 3, 4}); + +int min2 = std::min(1, std::min(2, std::min(3, 4))); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({1, 2, 3, 4}) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int min2 = std::min({1, 2, 3, 4}); + +int max3 = std::max(std::max(4, 5), std::min(2, std::min(3, 1))); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({4, 5, std::min({2, 3, 1})}) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested std::min calls, use std::min({2, 3, 1}) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max3 = std::max({4, 5, std::min({2, 3, 1})}); + +int min3 = std::min(std::min(4, 5), std::max(2, std::max(3, 1))); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({4, 5, std::max({2, 3, 1})}) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested std::max calls, use std::max({2, 3, 1}) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int min3 = std::min({4, 5, std::max({2, 3, 1})}); + +int max4 = std::max(1,std::max(2,3, greater_than), less_than); +// CHECK-FIXES: int max4 = std::max(1,std::max(2,3, greater_than), less_than); + +int min4 = std::min(1,std::min(2,3, greater_than), less_than); +// CHECK-FIXES: int min4 = std::min(1,std::min(2,3, greater_than), less_than); + +int max5 = std::max(1,std::max(2,3), less_than); +// CHECK-FIXES: int max5 = std::max(1,std::max(2,3), less_than); + +int min5 = std::min(1,std::min(2,3), less_than); +// CHECK-FIXES: int min5 = std::min(1,std::min(2,3), less_than); + +int max6 = std::max(1,std::max(2,3, greater_than), greater_than); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({1, 2, 3}, greater_than) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max6 = std::max({1, 2, 3}, greater_than); + +int min6 = std::min(1,std::min(2,3, greater_than), greater_than); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({1, 2, 3}, greater_than) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int min6 = std::min({1, 2, 3}, greater_than); + +int max7 = std::max(1,std::max(2,3, fless_than), fgreater_than); +// CHECK-FIXES: int max7 = std::max(1,std::max(2,3, fless_than), fgreater_than); + +int min7 = std::min(1,std::min(2,3, fless_than), fgreater_than); +// CHECK-FIXES: int min7 = std::min(1,std::min(2,3, fless_than), fgreater_than); + +int max8 = std::max(1,std::max(2,3, fless_than), less_than); +// CHECK-FIXES: int max8 = std::max(1,std::max(2,3, fless_than), less_than) + +int min8 = std::min(1,std::min(2,3, fless_than), less_than); +// CHECK-FIXES: int min8 = std::min(1,std::min(2,3, fless_than), less_than); + +int max9 = std::max(1,std::max(2,3, fless_than), fless_than); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({1, 2, 3}, fless_than) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max9 = std::max({1, 2, 3}, fless_than); + +int min9 = std::min(1,std::min(2,3, fless_than), fless_than); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({1, 2, 3}, fless_than) instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int min9 = std::min({1, 2, 3}, fless_than); + +} \ No newline at end of file >From f2e8474f88706363929855c543d0c8a9a4d77226 Mon Sep 17 00:00:00 2001 From: sopy <cont...@sopy.one> Date: Wed, 20 Mar 2024 13:12:55 +0200 Subject: [PATCH 3/6] Squash: Apply Clang format fixes and ensure InnerCall uses std::min or std::max in replacement generation --- .../MinMaxUseInitializerListCheck.cpp | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp index ddeac5b1b3ac77..93abbe023d8748 100644 --- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp @@ -89,9 +89,9 @@ MinMaxUseInitializerListCheck::findArgs(const MatchFinder::MatchResult &Match, Result.Compare = nullptr; if (Call->getNumArgs() > 2) { - auto argIterator = Call->arguments().begin(); - std::advance(argIterator, 2); - Result.Compare = *argIterator; + auto ArgIterator = Call->arguments().begin(); + std::advance(ArgIterator, 2); + Result.Compare = *ArgIterator; } for (const Expr *Arg : Call->arguments()) { @@ -153,15 +153,20 @@ std::string MinMaxUseInitializerListCheck::generateReplacement( for (const Expr *Arg : Result.Args) { QualType ArgType = Arg->getType(); - // check if expression is std::min or std::max if (const auto *InnerCall = dyn_cast<CallExpr>(Arg)) { - if (InnerCall->getDirectCallee() && - InnerCall->getDirectCallee()->getNameAsString() != - TopCall->getDirectCallee()->getNameAsString()) { - FindArgsResult innerResult = findArgs(Match, InnerCall); - ReplacementText += generateReplacement(Match, InnerCall, innerResult) += - "})"; - continue; + if (InnerCall->getDirectCallee()) { + std::string InnerCallNameStr = + InnerCall->getDirectCallee()->getQualifiedNameAsString(); + + if (InnerCallNameStr != + TopCall->getDirectCallee()->getQualifiedNameAsString() && + (InnerCallNameStr == "std::min" || + InnerCallNameStr == "std::max")) { + FindArgsResult innerResult = findArgs(Match, InnerCall); + ReplacementText += + generateReplacement(Match, InnerCall, innerResult) + ", "; + continue; + } } } >From bbdf160e7d9b700514d545cf28d382cb2db14c7f Mon Sep 17 00:00:00 2001 From: sopy <cont...@sopy.one> Date: Wed, 27 Mar 2024 11:56:34 +0200 Subject: [PATCH 4/6] Addressing code smell issues and consider nested calls with initializer lists as arguments --- .../MinMaxUseInitializerListCheck.cpp | 176 +++++++++++------- .../modernize/MinMaxUseInitializerListCheck.h | 13 +- clang-tools-extra/docs/ReleaseNotes.rst | 2 +- .../min-max-use-initializer-list.rst | 2 +- .../min-max-use-initializer-list.cpp | 133 +++++++++++-- 5 files changed, 228 insertions(+), 98 deletions(-) diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp index 93abbe023d8748..baf3da851e7f40 100644 --- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "MinMaxUseInitializerListCheck.h" +#include "../utils/ASTUtils.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Lex/Lexer.h" @@ -15,6 +16,19 @@ using namespace clang::ast_matchers; namespace clang::tidy::modernize { +struct FindArgsResult { + const Expr *First; + const Expr *Last; + const Expr *Compare; + std::vector<const Expr *> Args; +}; + +static const FindArgsResult findArgs(const MatchFinder::MatchResult &Match, + const CallExpr *Call); +static const std::string +generateReplacement(const MatchFinder::MatchResult &Match, + const CallExpr *TopCall, const FindArgsResult &Result); + MinMaxUseInitializerListCheck::MinMaxUseInitializerListCheck( StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), @@ -28,29 +42,18 @@ void MinMaxUseInitializerListCheck::storeOptions( } void MinMaxUseInitializerListCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher( - callExpr( - callee(functionDecl(hasName("::std::max"))), - anyOf(hasArgument( - 0, callExpr(callee(functionDecl(hasName("::std::max"))))), - hasArgument( - 1, callExpr(callee(functionDecl(hasName("::std::max")))))), - unless( - hasParent(callExpr(callee(functionDecl(hasName("::std::max"))))))) - .bind("topCall"), - this); - - Finder->addMatcher( - callExpr( - callee(functionDecl(hasName("::std::min"))), - anyOf(hasArgument( - 0, callExpr(callee(functionDecl(hasName("::std::min"))))), - hasArgument( - 1, callExpr(callee(functionDecl(hasName("::std::min")))))), - unless( - hasParent(callExpr(callee(functionDecl(hasName("::std::min"))))))) - .bind("topCall"), - this); + auto createMatcher = [](const std::string &functionName) { + auto funcDecl = functionDecl(hasName(functionName)); + auto expr = callExpr(callee(funcDecl)); + + return callExpr(callee(funcDecl), + anyOf(hasArgument(0, expr), hasArgument(1, expr)), + unless(hasParent(expr))) + .bind("topCall"); + }; + + Finder->addMatcher(createMatcher("::std::max"), this); + Finder->addMatcher(createMatcher("::std::min"), this); } void MinMaxUseInitializerListCheck::registerPPCallbacks( @@ -60,75 +63,98 @@ void MinMaxUseInitializerListCheck::registerPPCallbacks( void MinMaxUseInitializerListCheck::check( const MatchFinder::MatchResult &Match) { - const CallExpr *TopCall = Match.Nodes.getNodeAs<CallExpr>("topCall"); - MinMaxUseInitializerListCheck::FindArgsResult Result = - findArgs(Match, TopCall); - if (!Result.First || !Result.Last || Result.Args.size() <= 2) { + const auto *TopCall = Match.Nodes.getNodeAs<CallExpr>("topCall"); + FindArgsResult Result = findArgs(Match, TopCall); + + if (Result.Args.size() <= 2) { return; } - std::string ReplacementText = generateReplacement(Match, TopCall, Result); + const std::string ReplacementText = + generateReplacement(Match, TopCall, Result); diag(TopCall->getBeginLoc(), - "do not use nested std::%0 calls, use %1 instead") + "do not use nested 'std::%0' calls, use '%1' instead") << TopCall->getDirectCallee()->getName() << ReplacementText << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(TopCall->getBeginLoc(), - TopCall->getEndLoc()), + CharSourceRange::getTokenRange(TopCall->getSourceRange()), ReplacementText) - << Inserter.createMainFileIncludeInsertion("<algorithm>"); + << Inserter.createIncludeInsertion( + Match.SourceManager->getFileID(TopCall->getBeginLoc()), + "<algorithm>"); } -MinMaxUseInitializerListCheck::FindArgsResult -MinMaxUseInitializerListCheck::findArgs(const MatchFinder::MatchResult &Match, - const CallExpr *Call) { +static const FindArgsResult findArgs(const MatchFinder::MatchResult &Match, + const CallExpr *Call) { FindArgsResult Result; Result.First = nullptr; Result.Last = nullptr; Result.Compare = nullptr; - if (Call->getNumArgs() > 2) { + if (Call->getNumArgs() == 3) { auto ArgIterator = Call->arguments().begin(); std::advance(ArgIterator, 2); Result.Compare = *ArgIterator; + } else { + auto ArgIterator = Call->arguments().begin(); + + if (const auto *InitListExpr = + dyn_cast<CXXStdInitializerListExpr>(*ArgIterator)) { + if (const auto *TempExpr = + dyn_cast<MaterializeTemporaryExpr>(InitListExpr->getSubExpr())) { + if (const auto *InitList = + dyn_cast<clang::InitListExpr>(TempExpr->getSubExpr())) { + for (const Expr *Init : InitList->inits()) { + Result.Args.push_back(Init); + } + Result.First = *ArgIterator; + Result.Last = *ArgIterator; + + std::advance(ArgIterator, 1); + if (ArgIterator != Call->arguments().end()) { + Result.Compare = *ArgIterator; + } + return Result; + } + } + } } for (const Expr *Arg : Call->arguments()) { if (!Result.First) Result.First = Arg; - const CallExpr *InnerCall = dyn_cast<CallExpr>(Arg); + if (Arg == Result.Compare) + continue; + + const auto *InnerCall = dyn_cast<CallExpr>(Arg->IgnoreParenImpCasts()); + + if (InnerCall) { + printf("InnerCall: %s\n", + InnerCall->getDirectCallee()->getQualifiedNameAsString().c_str()); + printf("Call: %s\n", + Call->getDirectCallee()->getQualifiedNameAsString().c_str()); + } + if (InnerCall && InnerCall->getDirectCallee() && - InnerCall->getDirectCallee()->getNameAsString() == - Call->getDirectCallee()->getNameAsString()) { + InnerCall->getDirectCallee()->getQualifiedNameAsString() == + Call->getDirectCallee()->getQualifiedNameAsString()) { FindArgsResult InnerResult = findArgs(Match, InnerCall); - bool processInnerResult = false; - - if (!Result.Compare && !InnerResult.Compare) - processInnerResult = true; - else if (Result.Compare && InnerResult.Compare && - Lexer::getSourceText(CharSourceRange::getTokenRange( - Result.Compare->getSourceRange()), - *Match.SourceManager, - Match.Context->getLangOpts()) == - Lexer::getSourceText( - CharSourceRange::getTokenRange( - InnerResult.Compare->getSourceRange()), - *Match.SourceManager, Match.Context->getLangOpts())) - processInnerResult = true; - - if (processInnerResult) { + const bool ProcessInnerResult = + (!Result.Compare && !InnerResult.Compare) || + utils::areStatementsIdentical(Result.Compare, InnerResult.Compare, + *Match.Context); + + if (ProcessInnerResult) { Result.Args.insert(Result.Args.end(), InnerResult.Args.begin(), InnerResult.Args.end()); + Result.Last = InnerResult.Last; continue; } } - if (Arg == Result.Compare) - continue; - Result.Args.push_back(Arg); Result.Last = Arg; } @@ -136,9 +162,16 @@ MinMaxUseInitializerListCheck::findArgs(const MatchFinder::MatchResult &Match, return Result; } -std::string MinMaxUseInitializerListCheck::generateReplacement( - const MatchFinder::MatchResult &Match, const CallExpr *TopCall, - const FindArgsResult Result) { +static const std::string +generateReplacement(const MatchFinder::MatchResult &Match, + const CallExpr *TopCall, const FindArgsResult &Result) { + + const QualType ResultType = TopCall->getDirectCallee() + ->getReturnType() + .getNonReferenceType() + .getUnqualifiedType() + .getCanonicalType(); + std::string ReplacementText = Lexer::getSourceText( CharSourceRange::getTokenRange( @@ -147,15 +180,16 @@ std::string MinMaxUseInitializerListCheck::generateReplacement( *Match.SourceManager, Match.Context->getLangOpts()) .str() + "{"; - const QualType ResultType = - TopCall->getDirectCallee()->getReturnType().getNonReferenceType(); for (const Expr *Arg : Result.Args) { - QualType ArgType = Arg->getType(); + const QualType ArgType = Arg->IgnoreParenImpCasts() + ->getType() + .getUnqualifiedType() + .getCanonicalType(); if (const auto *InnerCall = dyn_cast<CallExpr>(Arg)) { if (InnerCall->getDirectCallee()) { - std::string InnerCallNameStr = + const std::string InnerCallNameStr = InnerCall->getDirectCallee()->getQualifiedNameAsString(); if (InnerCallNameStr != @@ -163,15 +197,17 @@ std::string MinMaxUseInitializerListCheck::generateReplacement( (InnerCallNameStr == "std::min" || InnerCallNameStr == "std::max")) { FindArgsResult innerResult = findArgs(Match, InnerCall); - ReplacementText += - generateReplacement(Match, InnerCall, innerResult) + ", "; - continue; + if (innerResult.Args.size() > 2) { + ReplacementText += + generateReplacement(Match, InnerCall, innerResult) + ", "; + + continue; + } } } } - bool CastNeeded = - ArgType.getCanonicalType() != ResultType.getCanonicalType(); + const bool CastNeeded = ArgType != ResultType; if (CastNeeded) ReplacementText += "static_cast<" + ResultType.getAsString() + ">("; diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h index 43d95004511430..65907d79457834 100644 --- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h @@ -14,7 +14,7 @@ namespace clang::tidy::modernize { -/// Replaces chained ``std::min`` and ``std::max`` calls with a initializer list +/// Replaces nested ``std::min`` and ``std::max`` calls with an initializer list /// where applicable. /// /// For example: @@ -46,18 +46,7 @@ class MinMaxUseInitializerListCheck : public ClangTidyCheck { } private: - struct FindArgsResult { - const Expr *First; - const Expr *Last; - const Expr *Compare; - std::vector<const Expr *> Args; - }; utils::IncludeInserter Inserter; - FindArgsResult findArgs(const ast_matchers::MatchFinder::MatchResult &Match, - const CallExpr *Call); - std::string - generateReplacement(const ast_matchers::MatchFinder::MatchResult &Match, - const CallExpr *TopCall, const FindArgsResult Result); }; } // namespace clang::tidy::modernize diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 13d14fa9ccf0aa..851e1f8a389a4d 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -120,7 +120,7 @@ New checks - New :doc:`modernize-min-max-use-initializer-list <clang-tidy/checks/modernize/min-max-use-initializer-list>` check. - Replaces chained ``std::min`` and ``std::max`` calls with a initializer list + Replaces nested ``std::min`` and ``std::max`` calls with an initializer list where applicable. - New :doc:`modernize-use-designated-initializers diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst index 0748e180897948..a166ec4b98cbc7 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst @@ -3,7 +3,7 @@ modernize-min-max-use-initializer-list ====================================== -Replaces chained ``std::min`` and ``std::max`` calls with a initializer list where applicable. +Replaces nested ``std::min`` and ``std::max`` calls with an initializer list where applicable. For instance, consider the following code: 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 3e15774f7b3f14..931854286374ed 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 @@ -1,27 +1,96 @@ // RUN: %check_clang_tidy %s modernize-min-max-use-initializer-list %t // CHECK-FIXES: #include <algorithm> +namespace utils { +template <typename T> +T max(T a, T b) { + return (a < b) ? b : a; +} +} // namespace utils + namespace std { +template< class T > +struct initializer_list { + initializer_list()=default; + initializer_list(T*,int){} + const T* begin() const {return nullptr;} + const T* end() const {return nullptr;} +}; + +template<class ForwardIt> +ForwardIt min_element(ForwardIt first, ForwardIt last) +{ + if (first == last) + return last; + + ForwardIt smallest = first; + + while (++first != last) + if (*first < *smallest) + smallest = first; + + return smallest; +} + +template<class ForwardIt> +ForwardIt max_element(ForwardIt first, ForwardIt last) +{ + if (first == last) + return last; + + ForwardIt largest = first; + + while (++first != last) + if (*largest < *first) + largest = first; + + return largest; +} + template< class T > const T& max( const T& a, const T& b ) { return (a < b) ? b : a; }; +template< class T > +T max(std::initializer_list<T> ilist) +{ + return *std::max_element(ilist.begin(), ilist.end()); +} + template< class T, class Compare > const T& max( const T& a, const T& b, Compare comp ) { return (comp(a, b)) ? b : a; }; +template< class T, class Compare > +T max(std::initializer_list<T> ilist, Compare comp) { + return *std::max_element(ilist.begin(), ilist.end(), comp); +}; + template< class T > const T& min( const T& a, const T& b ) { return (b < a) ? b : a; }; +template< class T > +T min(std::initializer_list<T> ilist) +{ + return *std::min_element(ilist.begin(), ilist.end()); +} + + template< class T, class Compare > const T& min( const T& a, const T& b, Compare comp ) { return (comp(b, a)) ? b : a; }; -} + +template< class T, class Compare > +T min(std::initializer_list<T> ilist, Compare comp) { + return *std::min_element(ilist.begin(), ilist.end(), comp); +}; + +} // namespace std using namespace std; @@ -37,29 +106,45 @@ auto less_than = [](int a, int b) { return a < b; }; auto greater_than = [](int a, int b) { return a > b; }; int max1 = std::max(1, std::max(2, 3)); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({1, 2, 3}) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3})' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max1 = std::max({1, 2, 3}); int min1 = std::min(1, std::min(2, 3)); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({1, 2, 3}) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({1, 2, 3})' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min1 = std::min({1, 2, 3}); int max2 = std::max(1, std::max(2, std::max(3, 4))); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({1, 2, 3, 4}) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max2 = std::max({1, 2, 3, 4}); +int max2b = std::max(std::max(std::max(1, 2), std::max(3, 4)), std::max(std::max(5, 6), std::max(7, 8))); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4, 5, 6, 7, 8})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max2b = std::max({1, 2, 3, 4, 5, 6, 7, 8}); + +int max2c = std::max(std::max(1, std::max(2, 3)), 4); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max2c = std::max({1, 2, 3, 4}); + +int max2d = std::max(std::max({1, 2, 3}), 4); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max2d = std::max({1, 2, 3, 4}); + +int max2e = std::max(1, max(2, max(3, 4))); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max2e = std::max({1, 2, 3, 4}); + int min2 = std::min(1, std::min(2, std::min(3, 4))); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({1, 2, 3, 4}) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min2 = std::min({1, 2, 3, 4}); int max3 = std::max(std::max(4, 5), std::min(2, std::min(3, 1))); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({4, 5, std::min({2, 3, 1})}) instead [modernize-min-max-use-initializer-list] -// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested std::min calls, use std::min({2, 3, 1}) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({4, 5, std::min({2, 3, 1})})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested 'std::min' calls, use 'std::min({2, 3, 1})' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max3 = std::max({4, 5, std::min({2, 3, 1})}); int min3 = std::min(std::min(4, 5), std::max(2, std::max(3, 1))); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({4, 5, std::max({2, 3, 1})}) instead [modernize-min-max-use-initializer-list] -// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested std::max calls, use std::max({2, 3, 1}) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({4, 5, std::max({2, 3, 1})})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested 'std::max' calls, use 'std::max({2, 3, 1})' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min3 = std::min({4, 5, std::max({2, 3, 1})}); int max4 = std::max(1,std::max(2,3, greater_than), less_than); @@ -75,11 +160,11 @@ int min5 = std::min(1,std::min(2,3), less_than); // CHECK-FIXES: int min5 = std::min(1,std::min(2,3), less_than); int max6 = std::max(1,std::max(2,3, greater_than), greater_than); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({1, 2, 3}, greater_than) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3}, greater_than)' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max6 = std::max({1, 2, 3}, greater_than); int min6 = std::min(1,std::min(2,3, greater_than), greater_than); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({1, 2, 3}, greater_than) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({1, 2, 3}, greater_than)' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min6 = std::min({1, 2, 3}, greater_than); int max7 = std::max(1,std::max(2,3, fless_than), fgreater_than); @@ -95,11 +180,31 @@ int min8 = std::min(1,std::min(2,3, fless_than), less_than); // CHECK-FIXES: int min8 = std::min(1,std::min(2,3, fless_than), less_than); int max9 = std::max(1,std::max(2,3, fless_than), fless_than); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::max calls, use std::max({1, 2, 3}, fless_than) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3}, fless_than)' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max9 = std::max({1, 2, 3}, fless_than); int min9 = std::min(1,std::min(2,3, fless_than), fless_than); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested std::min calls, use std::min({1, 2, 3}, fless_than) instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({1, 2, 3}, fless_than)' instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min9 = std::min({1, 2, 3}, fless_than); -} \ No newline at end of file +int min10 = std::min(std::min(4, 5), std::max(2, utils::max(3, 1))); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::min' calls, use 'std::min({4, 5, std::max(2, utils::max(3, 1))})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int min10 = std::min({4, 5, std::max(2, utils::max(3, 1))}); + +int typecastTest = std::max(std::max<int>(0U, 0.0f), 0); +// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: do not use nested 'std::max' calls, use 'std::max({static_cast<int>(0U), static_cast<int>(0.0f), 0})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int typecastTest = std::max({static_cast<int>(0U), static_cast<int>(0.0f), 0}); + +int typecastTest1 = std::max(std::max<long>(0U, 0.0f), 0L); +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use 'std::max({static_cast<long>(0U), static_cast<long>(0.0f), 0L})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int typecastTest1 = std::max({static_cast<long>(0U), static_cast<long>(0.0f), 0L}); + +int typecastTest2 = std::max(std::max<int>(10U, 20.0f), 30); +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use 'std::max({static_cast<int>(10U), static_cast<int>(20.0f), 30})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int typecastTest2 = std::max({static_cast<int>(10U), static_cast<int>(20.0f), 30}); + +int typecastTest3 = std::max(std::max<int>(0U, std::max<int>(0.0f, 1.0f)), 0); +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use 'std::max({static_cast<int>(0U), static_cast<int>(0.0f), static_cast<int>(1.0f), 0})' instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int typecastTest3 = std::max({static_cast<int>(0U), static_cast<int>(0.0f), static_cast<int>(1.0f), 0}); + +} >From 33a78916f3185489896e05100dfdb2c4a06ef7b9 Mon Sep 17 00:00:00 2001 From: sopy <cont...@sopy.one> Date: Fri, 29 Mar 2024 03:39:40 +0200 Subject: [PATCH 5/6] Refactor generateReplacement to return FixItHints to avoid check conflicts --- .../MinMaxUseInitializerListCheck.cpp | 299 ++++++++++++------ .../min-max-use-initializer-list.cpp | 136 +++++--- 2 files changed, 298 insertions(+), 137 deletions(-) diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp index baf3da851e7f40..1cf047149bf937 100644 --- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp @@ -23,9 +23,20 @@ struct FindArgsResult { std::vector<const Expr *> Args; }; -static const FindArgsResult findArgs(const MatchFinder::MatchResult &Match, - const CallExpr *Call); -static const std::string +static const FindArgsResult findArgs(const CallExpr *Call); +static std::vector<std::pair<int, int>> +getCommentRanges(const std::string &source); +static bool +isPositionInComment(int position, + const std::vector<std::pair<int, int>> &commentRanges); +static void +removeCharacterFromSource(std::string &FunctionCallSource, + const std::vector<std::pair<int, int>> &CommentRanges, + char Character, const CallExpr *InnerCall, + std::vector<FixItHint> &Result, bool ReverseSearch); +static SourceLocation +getLocForEndOfToken(const Expr *expr, const MatchFinder::MatchResult &Match); +static const std::vector<FixItHint> generateReplacement(const MatchFinder::MatchResult &Match, const CallExpr *TopCall, const FindArgsResult &Result); @@ -42,18 +53,20 @@ void MinMaxUseInitializerListCheck::storeOptions( } void MinMaxUseInitializerListCheck::registerMatchers(MatchFinder *Finder) { - auto createMatcher = [](const std::string &functionName) { - auto funcDecl = functionDecl(hasName(functionName)); - auto expr = callExpr(callee(funcDecl)); - - return callExpr(callee(funcDecl), - anyOf(hasArgument(0, expr), hasArgument(1, expr)), - unless(hasParent(expr))) + auto CreateMatcher = [](const std::string &FunctionName) { + auto FuncDecl = functionDecl(hasName(FunctionName)); + auto Expression = callExpr(callee(FuncDecl)); + + return callExpr(callee(FuncDecl), + anyOf(hasArgument(0, Expression), + hasArgument(1, Expression), + hasArgument(0, cxxStdInitializerListExpr())), + unless(hasParent(Expression))) .bind("topCall"); }; - Finder->addMatcher(createMatcher("::std::max"), this); - Finder->addMatcher(createMatcher("::std::min"), this); + Finder->addMatcher(CreateMatcher("::std::max"), this); + Finder->addMatcher(CreateMatcher("::std::min"), this); } void MinMaxUseInitializerListCheck::registerPPCallbacks( @@ -65,28 +78,35 @@ void MinMaxUseInitializerListCheck::check( const MatchFinder::MatchResult &Match) { const auto *TopCall = Match.Nodes.getNodeAs<CallExpr>("topCall"); - FindArgsResult Result = findArgs(Match, TopCall); - if (Result.Args.size() <= 2) { + // if topcall in macro ignore + if (TopCall->getBeginLoc().isMacroID()) { return; } - const std::string ReplacementText = + FindArgsResult Result = findArgs(TopCall); + const std::vector<FixItHint> Replacement = generateReplacement(Match, TopCall, Result); - diag(TopCall->getBeginLoc(), - "do not use nested 'std::%0' calls, use '%1' instead") - << TopCall->getDirectCallee()->getName() << ReplacementText - << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(TopCall->getSourceRange()), - ReplacementText) + // if only changes are inserting '{' and '}' then ignore + if (Replacement.size() <= 2) { + return; + } + + const DiagnosticBuilder Diagnostic = + diag(TopCall->getBeginLoc(), + "do not use nested 'std::%0' calls, use initializer lists instead") + << TopCall->getDirectCallee()->getName() << Inserter.createIncludeInsertion( Match.SourceManager->getFileID(TopCall->getBeginLoc()), "<algorithm>"); + + for (const auto &FixIt : Replacement) { + Diagnostic << FixIt; + } } -static const FindArgsResult findArgs(const MatchFinder::MatchResult &Match, - const CallExpr *Call) { +static const FindArgsResult findArgs(const CallExpr *Call) { FindArgsResult Result; Result.First = nullptr; Result.Last = nullptr; @@ -128,108 +148,195 @@ static const FindArgsResult findArgs(const MatchFinder::MatchResult &Match, if (Arg == Result.Compare) continue; - const auto *InnerCall = dyn_cast<CallExpr>(Arg->IgnoreParenImpCasts()); + Result.Args.push_back(Arg); + Result.Last = Arg; + } - if (InnerCall) { - printf("InnerCall: %s\n", - InnerCall->getDirectCallee()->getQualifiedNameAsString().c_str()); - printf("Call: %s\n", - Call->getDirectCallee()->getQualifiedNameAsString().c_str()); - } + return Result; +} - if (InnerCall && InnerCall->getDirectCallee() && - InnerCall->getDirectCallee()->getQualifiedNameAsString() == - Call->getDirectCallee()->getQualifiedNameAsString()) { - FindArgsResult InnerResult = findArgs(Match, InnerCall); +static std::vector<std::pair<int, int>> +getCommentRanges(const std::string &source) { + std::vector<std::pair<int, int>> commentRanges; + bool inComment = false; + bool singleLineComment = false; + int start = -1; + for (unsigned long i = 0; i < source.size(); i++) { + switch (source[i]) { + case '/': + if (source[i + 1] == '/') + singleLineComment = true; + else if (source[i + 1] == '*') + inComment = true; + break; + case '*': + if (source[i + 1] == '/') + inComment = false; + break; + case '\n': + singleLineComment = false; + break; + } + if (inComment || singleLineComment) { + if (start == -1) + start = i; + } else if (start != -1) { + commentRanges.push_back({start, i}); + start = -1; + } + } + return commentRanges; +} - const bool ProcessInnerResult = - (!Result.Compare && !InnerResult.Compare) || - utils::areStatementsIdentical(Result.Compare, InnerResult.Compare, - *Match.Context); +static bool +isPositionInComment(int position, + const std::vector<std::pair<int, int>> &commentRanges) { + return std::any_of(commentRanges.begin(), commentRanges.end(), + [position](const auto &range) { + return position >= range.first && + position <= range.second; + }); +} - if (ProcessInnerResult) { - Result.Args.insert(Result.Args.end(), InnerResult.Args.begin(), - InnerResult.Args.end()); - Result.Last = InnerResult.Last; - continue; - } +static void +removeCharacterFromSource(std::string &FunctionCallSource, + const std::vector<std::pair<int, int>> &CommentRanges, + char Character, const CallExpr *InnerCall, + std::vector<FixItHint> &Result, bool ReverseSearch) { + size_t CurrentSearch = ReverseSearch ? FunctionCallSource.size() : 0; + while ((CurrentSearch = + ReverseSearch + ? FunctionCallSource.rfind(Character, CurrentSearch - 1) + : FunctionCallSource.find(Character, CurrentSearch + 1)) != + std::string::npos) { + if (!isPositionInComment(CurrentSearch, CommentRanges)) { + Result.push_back(FixItHint::CreateRemoval(CharSourceRange::getCharRange( + InnerCall->getSourceRange().getBegin().getLocWithOffset( + CurrentSearch), + InnerCall->getSourceRange().getBegin().getLocWithOffset( + CurrentSearch + 1)))); + break; } - - Result.Args.push_back(Arg); - Result.Last = Arg; } +} - return Result; +SourceLocation getLocForEndOfToken(const Expr *expr, + const MatchFinder::MatchResult &Match) { + return Lexer::getLocForEndOfToken(expr->getEndLoc(), 0, *Match.SourceManager, + Match.Context->getLangOpts()); } -static const std::string +static const std::vector<FixItHint> generateReplacement(const MatchFinder::MatchResult &Match, const CallExpr *TopCall, const FindArgsResult &Result) { + std::vector<FixItHint> FixItHints; const QualType ResultType = TopCall->getDirectCallee() ->getReturnType() .getNonReferenceType() .getUnqualifiedType() .getCanonicalType(); + const bool IsInitializerList = Result.First == Result.Last; - std::string ReplacementText = - Lexer::getSourceText( - CharSourceRange::getTokenRange( - TopCall->getBeginLoc(), - Result.First->getBeginLoc().getLocWithOffset(-1)), - *Match.SourceManager, Match.Context->getLangOpts()) - .str() + - "{"; + if (!IsInitializerList) + FixItHints.push_back( + FixItHint::CreateInsertion(Result.First->getBeginLoc(), "{")); for (const Expr *Arg : Result.Args) { - const QualType ArgType = Arg->IgnoreParenImpCasts() - ->getType() - .getUnqualifiedType() - .getCanonicalType(); + std::string ArgText = + Lexer::getSourceText( + CharSourceRange::getTokenRange(Arg->getSourceRange()), + *Match.SourceManager, Match.Context->getLangOpts()) + .str(); + + if (const auto InnerCall = dyn_cast<CallExpr>(Arg->IgnoreParenImpCasts())) { + const auto InnerResult = findArgs(InnerCall); + const auto InnerReplacement = + generateReplacement(Match, InnerCall, InnerResult); + if (InnerCall->getDirectCallee()->getQualifiedNameAsString() == + TopCall->getDirectCallee()->getQualifiedNameAsString() && + ((!Result.Compare && !InnerResult.Compare) || + utils::areStatementsIdentical(Result.Compare, InnerResult.Compare, + *Match.Context))) { + std::vector<std::pair<int, int>> CommentRanges = + getCommentRanges(ArgText); + + FixItHints.push_back( + FixItHint::CreateRemoval(CharSourceRange::getTokenRange( + InnerCall->getCallee()->getSourceRange()))); + + removeCharacterFromSource(ArgText, CommentRanges, '(', InnerCall, + FixItHints, false); + + if (InnerResult.First == InnerResult.Last) { + removeCharacterFromSource(ArgText, CommentRanges, '{', InnerCall, + FixItHints, false); + removeCharacterFromSource(ArgText, CommentRanges, '}', InnerCall, + FixItHints, true); + FixItHints.insert(FixItHints.end(), InnerReplacement.begin(), + InnerReplacement.end()); + } else { + FixItHints.insert(FixItHints.end(), InnerReplacement.begin() + 1, + InnerReplacement.end() - 1); + } - if (const auto *InnerCall = dyn_cast<CallExpr>(Arg)) { - if (InnerCall->getDirectCallee()) { - const std::string InnerCallNameStr = - InnerCall->getDirectCallee()->getQualifiedNameAsString(); - - if (InnerCallNameStr != - TopCall->getDirectCallee()->getQualifiedNameAsString() && - (InnerCallNameStr == "std::min" || - InnerCallNameStr == "std::max")) { - FindArgsResult innerResult = findArgs(Match, InnerCall); - if (innerResult.Args.size() > 2) { - ReplacementText += - generateReplacement(Match, InnerCall, innerResult) + ", "; - - continue; + if (InnerResult.Compare) { + bool CommentFound = false; + + int InnerCallStartOffset = InnerCall->getBeginLoc().getRawEncoding(); + int CompareStart = + getLocForEndOfToken(InnerResult.Last, Match).getRawEncoding() - + InnerCallStartOffset; + int LastEnd = + getLocForEndOfToken(InnerResult.Compare, Match).getRawEncoding() - + InnerCallStartOffset; + + for (const auto &Range : CommentRanges) { + + if (Range.first >= CompareStart && Range.second <= LastEnd) + CommentFound = true; + } + + if (CommentFound) { + removeCharacterFromSource(ArgText, CommentRanges, ',', InnerCall, + FixItHints, true); + + FixItHints.push_back( + FixItHint::CreateRemoval(CharSourceRange::getCharRange( + InnerResult.Compare->getBeginLoc(), + getLocForEndOfToken(InnerResult.Compare, Match)))); + } else { + FixItHints.push_back( + FixItHint::CreateRemoval(CharSourceRange::getCharRange( + getLocForEndOfToken(InnerResult.Last, Match), + getLocForEndOfToken(InnerResult.Compare, Match)))); } } - } - } - const bool CastNeeded = ArgType != ResultType; + removeCharacterFromSource(ArgText, CommentRanges, ')', InnerCall, + FixItHints, true); - if (CastNeeded) - ReplacementText += "static_cast<" + ResultType.getAsString() + ">("; + continue; + } + } - ReplacementText += Lexer::getSourceText( - CharSourceRange::getTokenRange(Arg->getSourceRange()), - *Match.SourceManager, Match.Context->getLangOpts()); + const QualType ArgType = Arg->IgnoreParenImpCasts() + ->getType() + .getUnqualifiedType() + .getCanonicalType(); - if (CastNeeded) - ReplacementText += ")"; - ReplacementText += ", "; - } - ReplacementText = ReplacementText.substr(0, ReplacementText.size() - 2) + "}"; - if (Result.Compare) { - ReplacementText += ", "; - ReplacementText += Lexer::getSourceText( - CharSourceRange::getTokenRange(Result.Compare->getSourceRange()), - *Match.SourceManager, Match.Context->getLangOpts()); + if (ArgType != ResultType) { + FixItHints.push_back(FixItHint::CreateReplacement( + Arg->getSourceRange(), + "static_cast<" + ResultType.getAsString() + ">(" + ArgText + ")")); + } } - ReplacementText += ")"; - return ReplacementText; + if (!IsInitializerList) + FixItHints.push_back(FixItHint::CreateInsertion( + getLocForEndOfToken(Result.Last, Match), "}")); + + return FixItHints; } } // namespace clang::tidy::modernize 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 931854286374ed..287683f6299ae6 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 @@ -32,6 +32,21 @@ ForwardIt min_element(ForwardIt first, ForwardIt last) return smallest; } +template<class ForwardIt, class Compare> +ForwardIt min_element(ForwardIt first, ForwardIt last, Compare comp) +{ + if (first == last) + return last; + + ForwardIt smallest = first; + + while (++first != last) + if (comp(*first, *smallest)) + smallest = first; + + return smallest; +} + template<class ForwardIt> ForwardIt max_element(ForwardIt first, ForwardIt last) { @@ -47,6 +62,21 @@ ForwardIt max_element(ForwardIt first, ForwardIt last) return largest; } +template<class ForwardIt, class Compare> +ForwardIt max_element(ForwardIt first, ForwardIt last, Compare comp) +{ + if (first == last) + return last; + + ForwardIt largest = first; + + while(++first != last) + if (comp(*largest, *first)) + largest = first; + + return largest; +} + template< class T > const T& max( const T& a, const T& b ) { return (a < b) ? b : a; @@ -106,105 +136,129 @@ auto less_than = [](int a, int b) { return a < b; }; auto greater_than = [](int a, int b) { return a > b; }; int max1 = std::max(1, std::max(2, 3)); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max1 = std::max({1, 2, 3}); int min1 = std::min(1, std::min(2, 3)); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({1, 2, 3})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min1 = std::min({1, 2, 3}); int max2 = std::max(1, std::max(2, std::max(3, 4))); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max2 = std::max({1, 2, 3, 4}); int max2b = std::max(std::max(std::max(1, 2), std::max(3, 4)), std::max(std::max(5, 6), std::max(7, 8))); -// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4, 5, 6, 7, 8})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max2b = std::max({1, 2, 3, 4, 5, 6, 7, 8}); int max2c = std::max(std::max(1, std::max(2, 3)), 4); -// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max2c = std::max({1, 2, 3, 4}); int max2d = std::max(std::max({1, 2, 3}), 4); -// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max2d = std::max({1, 2, 3, 4}); int max2e = std::max(1, max(2, max(3, 4))); -// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max2e = std::max({1, 2, 3, 4}); int min2 = std::min(1, std::min(2, std::min(3, 4))); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({1, 2, 3, 4})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min2 = std::min({1, 2, 3, 4}); int max3 = std::max(std::max(4, 5), std::min(2, std::min(3, 1))); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({4, 5, std::min({2, 3, 1})})' instead [modernize-min-max-use-initializer-list] -// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested 'std::min' calls, use 'std::min({2, 3, 1})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested 'std::min' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max3 = std::max({4, 5, std::min({2, 3, 1})}); int min3 = std::min(std::min(4, 5), std::max(2, std::max(3, 1))); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({4, 5, std::max({2, 3, 1})})' instead [modernize-min-max-use-initializer-list] -// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested 'std::max' calls, use 'std::max({2, 3, 1})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use initializer lists instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-2]]:37: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min3 = std::min({4, 5, std::max({2, 3, 1})}); -int max4 = std::max(1,std::max(2,3, greater_than), less_than); -// CHECK-FIXES: int max4 = std::max(1,std::max(2,3, greater_than), less_than); +int max4 = std::max(1, std::max(2, 3, greater_than), less_than); +// CHECK-FIXES: int max4 = std::max(1, std::max(2, 3, greater_than), less_than); -int min4 = std::min(1,std::min(2,3, greater_than), less_than); -// CHECK-FIXES: int min4 = std::min(1,std::min(2,3, greater_than), less_than); +int min4 = std::min(1, std::min(2, 3, greater_than), less_than); +// CHECK-FIXES: int min4 = std::min(1, std::min(2, 3, greater_than), less_than); -int max5 = std::max(1,std::max(2,3), less_than); -// CHECK-FIXES: int max5 = std::max(1,std::max(2,3), less_than); +int max5 = std::max(1, std::max(2, 3), less_than); +// CHECK-FIXES: int max5 = std::max(1, std::max(2, 3), less_than); -int min5 = std::min(1,std::min(2,3), less_than); -// CHECK-FIXES: int min5 = std::min(1,std::min(2,3), less_than); +int min5 = std::min(1, std::min(2, 3), less_than); +// CHECK-FIXES: int min5 = std::min(1, std::min(2, 3), less_than); -int max6 = std::max(1,std::max(2,3, greater_than), greater_than); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3}, greater_than)' instead [modernize-min-max-use-initializer-list] +int max6 = std::max(1, std::max(2, 3, greater_than), greater_than); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max6 = std::max({1, 2, 3}, greater_than); -int min6 = std::min(1,std::min(2,3, greater_than), greater_than); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({1, 2, 3}, greater_than)' instead [modernize-min-max-use-initializer-list] +int min6 = std::min(1, std::min(2, 3, greater_than), greater_than); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min6 = std::min({1, 2, 3}, greater_than); -int max7 = std::max(1,std::max(2,3, fless_than), fgreater_than); -// CHECK-FIXES: int max7 = std::max(1,std::max(2,3, fless_than), fgreater_than); +int max7 = std::max(1, std::max(2, 3, fless_than), fgreater_than); +// CHECK-FIXES: int max7 = std::max(1, std::max(2, 3, fless_than), fgreater_than); -int min7 = std::min(1,std::min(2,3, fless_than), fgreater_than); -// CHECK-FIXES: int min7 = std::min(1,std::min(2,3, fless_than), fgreater_than); +int min7 = std::min(1, std::min(2, 3, fless_than), fgreater_than); +// CHECK-FIXES: int min7 = std::min(1, std::min(2, 3, fless_than), fgreater_than); -int max8 = std::max(1,std::max(2,3, fless_than), less_than); -// CHECK-FIXES: int max8 = std::max(1,std::max(2,3, fless_than), less_than) +int max8 = std::max(1, std::max(2, 3, fless_than), less_than); +// CHECK-FIXES: int max8 = std::max(1, std::max(2, 3, fless_than), less_than) -int min8 = std::min(1,std::min(2,3, fless_than), less_than); -// CHECK-FIXES: int min8 = std::min(1,std::min(2,3, fless_than), less_than); +int min8 = std::min(1, std::min(2, 3, fless_than), less_than); +// CHECK-FIXES: int min8 = std::min(1, std::min(2, 3, fless_than), less_than); -int max9 = std::max(1,std::max(2,3, fless_than), fless_than); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use 'std::max({1, 2, 3}, fless_than)' instead [modernize-min-max-use-initializer-list] +int max9 = std::max(1, std::max(2, 3, fless_than), fless_than); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int max9 = std::max({1, 2, 3}, fless_than); -int min9 = std::min(1,std::min(2,3, fless_than), fless_than); -// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use 'std::min({1, 2, 3}, fless_than)' instead [modernize-min-max-use-initializer-list] +int min9 = std::min(1, std::min(2, 3, fless_than), fless_than); +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min9 = std::min({1, 2, 3}, fless_than); int min10 = std::min(std::min(4, 5), std::max(2, utils::max(3, 1))); -// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::min' calls, use 'std::min({4, 5, std::max(2, utils::max(3, 1))})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::min' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int min10 = std::min({4, 5, std::max(2, utils::max(3, 1))}); +int max10 = std::max({std::max(1, 2), std::max({5, 6, 1}), 2, std::min({1, 2, 4})}); +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int max10 = std::max({1, 2, 5, 6, 1, 2, std::min({1, 2, 4})}); + int typecastTest = std::max(std::max<int>(0U, 0.0f), 0); -// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: do not use nested 'std::max' calls, use 'std::max({static_cast<int>(0U), static_cast<int>(0.0f), 0})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int typecastTest = std::max({static_cast<int>(0U), static_cast<int>(0.0f), 0}); int typecastTest1 = std::max(std::max<long>(0U, 0.0f), 0L); -// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use 'std::max({static_cast<long>(0U), static_cast<long>(0.0f), 0L})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int typecastTest1 = std::max({static_cast<long>(0U), static_cast<long>(0.0f), 0L}); int typecastTest2 = std::max(std::max<int>(10U, 20.0f), 30); -// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use 'std::max({static_cast<int>(10U), static_cast<int>(20.0f), 30})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int typecastTest2 = std::max({static_cast<int>(10U), static_cast<int>(20.0f), 30}); int typecastTest3 = std::max(std::max<int>(0U, std::max<int>(0.0f, 1.0f)), 0); -// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use 'std::max({static_cast<int>(0U), static_cast<int>(0.0f), static_cast<int>(1.0f), 0})' instead [modernize-min-max-use-initializer-list] +// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int typecastTest3 = std::max({static_cast<int>(0U), static_cast<int>(0.0f), static_cast<int>(1.0f), 0}); +# define max3(a, b, c) std::max(a, std::max(b, c)) +// CHECK-FIXES: # define max3(a, b, c) std::max(a, std::max(b, c)) + +#define custom_max(a, b) std::max(a, b) +// CHECK-FIXES: #define custom_max(a, b) std::max(a, b) + +#define value 4545 +int macroVarMax = std::max(value, std::max(1, 2)); +// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int macroVarMax = std::max({value, 1, 2}); + +#define value2 45U +int macroVarMax2 = std::max(1, std::max<int>(value2, 2.0f)); +// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int macroVarMax2 = std::max({1, static_cast<int>(value2), static_cast<int>(2.0f)}); + +int minWithComments = std::min(std::min/*(*/(/*{*/{4, 5/*}*/},/*,*/ greater_than)/*)*/, 1, greater_than); +// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: do not use nested 'std::min' calls, use initializer lists instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: int minWithComments = std::min({/*(*//*{*/4, 5/*}*//*,*/ /*)*/, 1}, greater_than); + } >From f0b06c9addd0a866ebd5567250a4fb9074f6621b Mon Sep 17 00:00:00 2001 From: sopy <cont...@sopy.one> Date: Sat, 30 Mar 2024 18:54:29 +0200 Subject: [PATCH 6/6] Undo ignore if in macro --- .../modernize/MinMaxUseInitializerListCheck.cpp | 5 ----- .../checkers/modernize/min-max-use-initializer-list.cpp | 8 +++----- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp index 1cf047149bf937..760809ec0dc032 100644 --- a/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp @@ -79,11 +79,6 @@ void MinMaxUseInitializerListCheck::check( const auto *TopCall = Match.Nodes.getNodeAs<CallExpr>("topCall"); - // if topcall in macro ignore - if (TopCall->getBeginLoc().isMacroID()) { - return; - } - FindArgsResult Result = findArgs(TopCall); const std::vector<FixItHint> Replacement = generateReplacement(Match, TopCall, Result); 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 287683f6299ae6..a2ea02efcb3b5a 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 @@ -241,11 +241,9 @@ int typecastTest3 = std::max(std::max<int>(0U, std::max<int>(0.0f, 1.0f)), 0); // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] // CHECK-FIXES: int typecastTest3 = std::max({static_cast<int>(0U), static_cast<int>(0.0f), static_cast<int>(1.0f), 0}); -# define max3(a, b, c) std::max(a, std::max(b, c)) -// CHECK-FIXES: # define max3(a, b, c) std::max(a, std::max(b, c)) - -#define custom_max(a, b) std::max(a, b) -// CHECK-FIXES: #define custom_max(a, b) std::max(a, b) +#define max3(a, b, c) std::max(a, std::max(b, c)) +// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: do not use nested 'std::max' calls, use initializer lists instead [modernize-min-max-use-initializer-list] +// CHECK-FIXES: # define max3(a, b, c) std::max({a, b, c}) #define value 4545 int macroVarMax = std::max(value, std::max(1, 2)); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
