Skip to content

Commit 7e8b2d9

Browse files
sopybsopyb
committed
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
1 parent 7f0e9d1 commit 7e8b2d9

File tree

5 files changed

+270
-76
lines changed

5 files changed

+270
-76
lines changed

‎clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.cpp

Lines changed: 116 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -31,19 +31,25 @@ void MinMaxUseInitializerListCheck::registerMatchers(MatchFinder *Finder) {
3131
Finder->addMatcher(
3232
callExpr(
3333
callee(functionDecl(hasName("::std::max"))),
34-
hasAnyArgument(callExpr(callee(functionDecl(hasName("::std::max"))))),
34+
anyOf(hasArgument(
35+
0, callExpr(callee(functionDecl(hasName("::std::max"))))),
36+
hasArgument(
37+
1, callExpr(callee(functionDecl(hasName("::std::max")))))),
3538
unless(
3639
hasParent(callExpr(callee(functionDecl(hasName("::std::max")))))))
37-
.bind("maxCall"),
40+
.bind("topCall"),
3841
this);
3942

4043
Finder->addMatcher(
4144
callExpr(
4245
callee(functionDecl(hasName("::std::min"))),
43-
hasAnyArgument(callExpr(callee(functionDecl(hasName("::std::min"))))),
46+
anyOf(hasArgument(
47+
0, callExpr(callee(functionDecl(hasName("::std::min"))))),
48+
hasArgument(
49+
1, callExpr(callee(functionDecl(hasName("::std::min")))))),
4450
unless(
4551
hasParent(callExpr(callee(functionDecl(hasName("::std::min")))))))
46-
.bind("minCall"),
52+
.bind("topCall"),
4753
this);
4854
}
4955

@@ -53,29 +59,112 @@ void MinMaxUseInitializerListCheck::registerPPCallbacks(
5359
}
5460

5561
void MinMaxUseInitializerListCheck::check(
56-
const MatchFinder::MatchResult &Result) {
57-
const auto *MaxCall = Result.Nodes.getNodeAs<CallExpr>("maxCall");
58-
const auto *MinCall = Result.Nodes.getNodeAs<CallExpr>("minCall");
62+
const MatchFinder::MatchResult &Match) {
63+
const CallExpr *TopCall = Match.Nodes.getNodeAs<CallExpr>("topCall");
64+
MinMaxUseInitializerListCheck::FindArgsResult Result =
65+
findArgs(Match, TopCall);
5966

60-
const CallExpr *TopCall = MaxCall ? MaxCall : MinCall;
61-
if (!TopCall) {
67+
if (!Result.First || !Result.Last || Result.Args.size() <= 2) {
6268
return;
6369
}
64-
const QualType ResultType =
65-
TopCall->getDirectCallee()->getReturnType().getNonReferenceType();
6670

67-
const Expr *FirstArg = nullptr;
68-
const Expr *LastArg = nullptr;
69-
std::vector<const Expr *> Args;
70-
findArgs(TopCall, &FirstArg, &LastArg, Args);
71+
std::string ReplacementText = generateReplacement(Match, TopCall, Result);
7172

72-
if (!FirstArg || !LastArg || Args.size() <= 2) {
73-
return;
73+
diag(TopCall->getBeginLoc(),
74+
"do not use nested std::%0 calls, use %1 instead")
75+
<< TopCall->getDirectCallee()->getName() << ReplacementText
76+
<< FixItHint::CreateReplacement(
77+
CharSourceRange::getTokenRange(TopCall->getBeginLoc(),
78+
TopCall->getEndLoc()),
79+
ReplacementText)
80+
<< Inserter.createMainFileIncludeInsertion("<algorithm>");
81+
}
82+
83+
MinMaxUseInitializerListCheck::FindArgsResult
84+
MinMaxUseInitializerListCheck::findArgs(const MatchFinder::MatchResult &Match,
85+
const CallExpr *Call) {
86+
FindArgsResult Result;
87+
Result.First = nullptr;
88+
Result.Last = nullptr;
89+
Result.Compare = nullptr;
90+
91+
if (Call->getNumArgs() > 2) {
92+
auto argIterator = Call->arguments().begin();
93+
std::advance(argIterator, 2);
94+
Result.Compare = *argIterator;
7495
}
7596

76-
std::string ReplacementText = "{";
77-
for (const Expr *Arg : Args) {
97+
for (const Expr *Arg : Call->arguments()) {
98+
if (!Result.First)
99+
Result.First = Arg;
100+
101+
const CallExpr *InnerCall = dyn_cast<CallExpr>(Arg);
102+
if (InnerCall && InnerCall->getDirectCallee() &&
103+
InnerCall->getDirectCallee()->getNameAsString() ==
104+
Call->getDirectCallee()->getNameAsString()) {
105+
FindArgsResult InnerResult = findArgs(Match, InnerCall);
106+
107+
bool processInnerResult = false;
108+
109+
if (!Result.Compare && !InnerResult.Compare)
110+
processInnerResult = true;
111+
else if (Result.Compare && InnerResult.Compare &&
112+
Lexer::getSourceText(CharSourceRange::getTokenRange(
113+
Result.Compare->getSourceRange()),
114+
*Match.SourceManager,
115+
Match.Context->getLangOpts()) ==
116+
Lexer::getSourceText(
117+
CharSourceRange::getTokenRange(
118+
InnerResult.Compare->getSourceRange()),
119+
*Match.SourceManager, Match.Context->getLangOpts()))
120+
processInnerResult = true;
121+
122+
if (processInnerResult) {
123+
Result.Args.insert(Result.Args.end(), InnerResult.Args.begin(),
124+
InnerResult.Args.end());
125+
continue;
126+
}
127+
}
128+
129+
if (Arg == Result.Compare)
130+
continue;
131+
132+
Result.Args.push_back(Arg);
133+
Result.Last = Arg;
134+
}
135+
136+
return Result;
137+
}
138+
139+
std::string MinMaxUseInitializerListCheck::generateReplacement(
140+
const MatchFinder::MatchResult &Match, const CallExpr *TopCall,
141+
const FindArgsResult Result) {
142+
std::string ReplacementText =
143+
Lexer::getSourceText(
144+
CharSourceRange::getTokenRange(
145+
TopCall->getBeginLoc(),
146+
Result.First->getBeginLoc().getLocWithOffset(-1)),
147+
*Match.SourceManager, Match.Context->getLangOpts())
148+
.str() +
149+
"{";
150+
const QualType ResultType =
151+
TopCall->getDirectCallee()->getReturnType().getNonReferenceType();
152+
153+
for (const Expr *Arg : Result.Args) {
78154
QualType ArgType = Arg->getType();
155+
156+
// check if expression is std::min or std::max
157+
if (const auto *InnerCall = dyn_cast<CallExpr>(Arg)) {
158+
if (InnerCall->getDirectCallee() &&
159+
InnerCall->getDirectCallee()->getNameAsString() !=
160+
TopCall->getDirectCallee()->getNameAsString()) {
161+
FindArgsResult innerResult = findArgs(Match, InnerCall);
162+
ReplacementText += generateReplacement(Match, InnerCall, innerResult) +=
163+
"})";
164+
continue;
165+
}
166+
}
167+
79168
bool CastNeeded =
80169
ArgType.getCanonicalType() != ResultType.getCanonicalType();
81170

@@ -84,55 +173,22 @@ void MinMaxUseInitializerListCheck::check(
84173

85174
ReplacementText += Lexer::getSourceText(
86175
CharSourceRange::getTokenRange(Arg->getSourceRange()),
87-
*Result.SourceManager, Result.Context->getLangOpts());
176+
*Match.SourceManager, Match.Context->getLangOpts());
88177

89178
if (CastNeeded)
90179
ReplacementText += ")";
91180
ReplacementText += ", ";
92181
}
93182
ReplacementText = ReplacementText.substr(0, ReplacementText.size() - 2) + "}";
94-
95-
diag(TopCall->getBeginLoc(),
96-
"do not use nested std::%0 calls, use %1 instead")
97-
<< TopCall->getDirectCallee()->getName() << ReplacementText
98-
<< FixItHint::CreateReplacement(
99-
CharSourceRange::getTokenRange(
100-
FirstArg->getBeginLoc(),
101-
Lexer::getLocForEndOfToken(TopCall->getEndLoc(), 0,
102-
Result.Context->getSourceManager(),
103-
Result.Context->getLangOpts())
104-
.getLocWithOffset(-2)),
105-
ReplacementText)
106-
<< Inserter.createMainFileIncludeInsertion("<algorithm>");
107-
}
108-
109-
void MinMaxUseInitializerListCheck::findArgs(const CallExpr *Call,
110-
const Expr **First,
111-
const Expr **Last,
112-
std::vector<const Expr *> &Args) {
113-
if (!Call) {
114-
return;
115-
}
116-
117-
const FunctionDecl *Callee = Call->getDirectCallee();
118-
if (!Callee) {
119-
return;
183+
if (Result.Compare) {
184+
ReplacementText += ", ";
185+
ReplacementText += Lexer::getSourceText(
186+
CharSourceRange::getTokenRange(Result.Compare->getSourceRange()),
187+
*Match.SourceManager, Match.Context->getLangOpts());
120188
}
189+
ReplacementText += ")";
121190

122-
for (const Expr *Arg : Call->arguments()) {
123-
if (!*First)
124-
*First = Arg;
125-
126-
const CallExpr *InnerCall = dyn_cast<CallExpr>(Arg);
127-
if (InnerCall && InnerCall->getDirectCallee() &&
128-
InnerCall->getDirectCallee()->getNameAsString() ==
129-
Call->getDirectCallee()->getNameAsString()) {
130-
findArgs(InnerCall, First, Last, Args);
131-
} else
132-
Args.push_back(Arg);
133-
134-
*Last = Arg;
135-
}
191+
return ReplacementText;
136192
}
137193

138194
} // namespace clang::tidy::modernize

‎clang-tools-extra/clang-tidy/modernize/MinMaxUseInitializerListCheck.h

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,9 @@
1414

1515
namespace clang::tidy::modernize {
1616

17-
/// Transforms the repeated calls to `std::min` and `std::max` into a single
18-
/// call using initializer lists.
17+
/// Replaces chained ``std::min`` and ``std::max`` calls with a initializer list
18+
/// where applicable.
1919
///
20-
/// It identifies cases where `std::min` or `std::max` is used to find the
21-
/// minimum or maximum value among more than two items through repeated calls.
22-
/// The check replaces these calls with a single call to `std::min` or
23-
/// `std::max` that uses an initializer list. This makes the code slightly more
24-
/// efficient.
25-
/// \n\n
2620
/// For example:
2721
///
2822
/// \code
@@ -38,19 +32,32 @@ class MinMaxUseInitializerListCheck : public ClangTidyCheck {
3832
public:
3933
MinMaxUseInitializerListCheck(StringRef Name, ClangTidyContext *Context);
4034

41-
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
42-
return LangOpts.CPlusPlus11;
43-
}
4435
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
4536
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
4637
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
4738
Preprocessor *ModuleExpanderPP) override;
48-
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
39+
void check(const ast_matchers::MatchFinder::MatchResult &Match) override;
40+
41+
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
42+
return LangOpts.CPlusPlus11;
43+
}
44+
std::optional<TraversalKind> getCheckTraversalKind() const override {
45+
return TK_IgnoreUnlessSpelledInSource;
46+
}
4947

5048
private:
49+
struct FindArgsResult {
50+
const Expr *First;
51+
const Expr *Last;
52+
const Expr *Compare;
53+
std::vector<const Expr *> Args;
54+
};
5155
utils::IncludeInserter Inserter;
52-
void findArgs(const CallExpr *call, const Expr **first, const Expr **last,
53-
std::vector<const Expr *> &args);
56+
FindArgsResult findArgs(const ast_matchers::MatchFinder::MatchResult &Match,
57+
const CallExpr *Call);
58+
std::string
59+
generateReplacement(const ast_matchers::MatchFinder::MatchResult &Match,
60+
const CallExpr *TopCall, const FindArgsResult Result);
5461
};
5562

5663
} // namespace clang::tidy::modernize

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,8 @@ New checks
120120
- New :doc:`modernize-min-max-use-initializer-list
121121
<clang-tidy/checks/modernize/min-max-use-initializer-list>` check.
122122

123-
Replaces chained ``std::min`` and ``std::max`` calls that can be written as
124-
initializer lists.
123+
Replaces chained ``std::min`` and ``std::max`` calls with a initializer list
124+
where applicable.
125125

126126
- New :doc:`modernize-use-designated-initializers
127127
<clang-tidy/checks/modernize/use-designated-initializers>` check.

‎clang-tools-extra/docs/clang-tidy/checks/modernize/min-max-use-initializer-list.rst

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
.. title:: clang-tidy - modernize-min-max-use-initializer-list
2+
3+
modernize-min-max-use-initializer-list
4+
======================================
5+
6+
Replaces chained ``std::min`` and ``std::max`` calls with a initializer list where applicable.
7+
8+
For instance, consider the following code:
9+
10+
.. code-block:: cpp
11+
12+
int a = std::max(std::max(i, j), k);
13+
14+
`modernize-min-max-use-initializer-list` check will transform the above code to:
15+
16+
.. code-block:: cpp
17+
18+
int a = std::max({i, j, k});
19+
20+
Options
21+
=======
22+
23+
.. option:: IncludeStyle
24+
25+
A string specifying which include-style is used, `llvm` or `google`. Default
26+
is `llvm`.

0 commit comments

Comments
 (0)