Skip to content

Commit 65079e0

Browse files
sopybsopyb
committed
Addressing issues raised in review
1 parent 2126e62 commit 65079e0

File tree

2 files changed

+53
-66
lines changed

2 files changed

+53
-66
lines changed

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

Lines changed: 39 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -48,35 +48,30 @@ static FindArgsResult findArgs(const CallExpr *Call) {
4848
InitListExpr->getSubExpr()->IgnoreImplicit())
4949
: nullptr;
5050

51-
if (InitListExpr && InitList) {
52-
Result.Args.insert(Result.Args.begin(), InitList->inits().begin(),
53-
InitList->inits().end());
51+
if (InitList) {
52+
Result.Args.append(InitList->inits().begin(), InitList->inits().end());
5453
Result.First = *ArgIterator;
5554
Result.Last = *ArgIterator;
5655

5756
// check if there is a comparison argument
5857
std::advance(ArgIterator, 1);
59-
if (ArgIterator != Call->arguments().end()) {
58+
if (ArgIterator != Call->arguments().end())
6059
Result.Compare = *ArgIterator;
61-
}
6260

6361
return Result;
6462
}
65-
// if it has 3 arguments then the last will be the comparison
6663
} else {
64+
// if it has 3 arguments then the last will be the comparison
6765
Result.Compare = *(std::next(Call->arguments().begin(), 2));
6866
}
6967

70-
for (const Expr *Arg : Call->arguments()) {
71-
if (!Result.First)
72-
Result.First = Arg;
68+
if (Result.Compare)
69+
Result.Args = SmallVector<const Expr *>(llvm::drop_end(Call->arguments()));
70+
else
71+
Result.Args = SmallVector<const Expr *>(Call->arguments());
7372

74-
if (Arg == Result.Compare)
75-
continue;
76-
77-
Result.Args.push_back(Arg);
78-
Result.Last = Arg;
79-
}
73+
Result.First = Result.Args.front();
74+
Result.Last = Result.Args.back();
8075

8176
return Result;
8277
}
@@ -91,24 +86,8 @@ generateReplacement(const MatchFinder::MatchResult &Match,
9186
.getNonReferenceType()
9287
.getUnqualifiedType()
9388
.getCanonicalType();
94-
const auto &SourceMngr = *Match.SourceManager;
95-
const auto LanguageOpts = Match.Context->getLangOpts();
96-
const bool IsInitializerList = Result.First == Result.Last;
97-
98-
// add { and } if the top call doesn't have an initializer list arg
99-
if (!IsInitializerList) {
100-
FixItHints.push_back(
101-
FixItHint::CreateInsertion(Result.First->getBeginLoc(), "{"));
102-
103-
if (Result.Compare)
104-
FixItHints.push_back(FixItHint::CreateInsertion(
105-
Lexer::getLocForEndOfToken(Result.Last->getEndLoc(), 0, SourceMngr,
106-
LanguageOpts),
107-
"}"));
108-
else
109-
FixItHints.push_back(
110-
FixItHint::CreateInsertion(TopCall->getEndLoc(), "}"));
111-
}
89+
const SourceManager &SourceMngr = *Match.SourceManager;
90+
const LangOptions &LanguageOpts = Match.Context->getLangOpts();
11291

11392
for (const Expr *Arg : Result.Args) {
11493
const auto *InnerCall = dyn_cast<CallExpr>(Arg->IgnoreParenImpCasts());
@@ -140,10 +119,7 @@ generateReplacement(const MatchFinder::MatchResult &Match,
140119
continue;
141120
}
142121

143-
const auto InnerResult = findArgs(InnerCall);
144-
const auto InnerReplacements =
145-
generateReplacement(Match, InnerCall, InnerResult);
146-
const bool IsInnerInitializerList = InnerResult.First == InnerResult.Last;
122+
const FindArgsResult InnerResult = findArgs(InnerCall);
147123

148124
// if the nested call doesn't have arguments skip it
149125
if (!InnerResult.First || !InnerResult.Last)
@@ -162,8 +138,7 @@ generateReplacement(const MatchFinder::MatchResult &Match,
162138

163139
// remove the function call
164140
FixItHints.push_back(
165-
FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
166-
InnerCall->getCallee()->getSourceRange())));
141+
FixItHint::CreateRemoval(InnerCall->getCallee()->getSourceRange()));
167142

168143
// remove the parentheses
169144
const auto LParen = utils::lexer::findNextTokenSkippingComments(
@@ -174,41 +149,33 @@ generateReplacement(const MatchFinder::MatchResult &Match,
174149
FixItHint::CreateRemoval(SourceRange(InnerCall->getRParenLoc())));
175150

176151
// if the inner call has an initializer list arg
177-
if (IsInnerInitializerList) {
152+
if (InnerResult.First == InnerResult.Last) {
178153
// remove the initializer list braces
179154
FixItHints.push_back(FixItHint::CreateRemoval(
180155
CharSourceRange::getTokenRange(InnerResult.First->getBeginLoc())));
181156
FixItHints.push_back(FixItHint::CreateRemoval(
182157
CharSourceRange::getTokenRange(InnerResult.First->getEndLoc())));
183158
}
184159

160+
const SmallVector<FixItHint> InnerReplacements =
161+
generateReplacement(Match, InnerCall, InnerResult);
162+
185163
FixItHints.insert(FixItHints.end(),
186164
// ignore { and } insertions for the inner call if it does
187165
// not have an initializer list arg
188-
InnerReplacements.begin() + (!IsInnerInitializerList) * 2,
189-
InnerReplacements.end());
166+
InnerReplacements.begin(), InnerReplacements.end());
190167

191168
if (InnerResult.Compare) {
192169
// find the comma after the value arguments
193170
const auto Comma = utils::lexer::findNextTokenSkippingComments(
194171
InnerResult.Last->getEndLoc(), SourceMngr, LanguageOpts);
195172

196-
// if there are comments between the comma and the comparison
197-
if (utils::lexer::getPreviousToken(InnerResult.Compare->getExprLoc(),
198-
SourceMngr, LanguageOpts, false)
199-
.getLocation() != Comma->getLocation()) {
200-
// remove the comma and the comparison
201-
FixItHints.push_back(
202-
FixItHint::CreateRemoval(SourceRange(Comma->getLocation())));
203-
204-
FixItHints.push_back(
205-
FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
206-
InnerResult.Compare->getSourceRange())));
207-
} else
208-
// remove everything after the last argument
209-
FixItHints.push_back(
210-
FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
211-
Comma->getLocation(), InnerResult.Compare->getEndLoc())));
173+
// remove the comma and the comparison
174+
FixItHints.push_back(
175+
FixItHint::CreateRemoval(SourceRange(Comma->getLocation())));
176+
177+
FixItHints.push_back(
178+
FixItHint::CreateRemoval(InnerResult.Compare->getSourceRange()));
212179
}
213180
}
214181

@@ -258,9 +225,8 @@ void MinMaxUseInitializerListCheck::check(
258225
const SmallVector<FixItHint> Replacement =
259226
generateReplacement(Match, TopCall, Result);
260227

261-
if (Replacement.size() <= 2) {
228+
if (Replacement.empty())
262229
return;
263-
}
264230

265231
const DiagnosticBuilder Diagnostic =
266232
diag(TopCall->getBeginLoc(),
@@ -270,9 +236,20 @@ void MinMaxUseInitializerListCheck::check(
270236
Match.SourceManager->getFileID(TopCall->getBeginLoc()),
271237
"<algorithm>");
272238

273-
for (const auto &FixIt : Replacement) {
274-
Diagnostic << FixIt;
239+
// if the top call doesn't have an initializer list argument
240+
if (Result.First != Result.Last) {
241+
// add { and } insertions
242+
Diagnostic << FixItHint::CreateInsertion(Result.First->getBeginLoc(), "{");
243+
244+
Diagnostic << FixItHint::CreateInsertion(
245+
Lexer::getLocForEndOfToken(Result.Last->getEndLoc(), 0,
246+
*Match.SourceManager,
247+
Match.Context->getLangOpts()),
248+
"}");
275249
}
250+
251+
for (const auto &FixIt : Replacement)
252+
Diagnostic << FixIt;
276253
}
277254

278255
} // namespace clang::tidy::modernize

‎clang-tools-extra/test/clang-tidy/checkers/modernize/min-max-use-initializer-list.cpp

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -192,11 +192,11 @@ int min5 = std::min(1, std::min(2, 3), less_than);
192192

193193
int max6 = std::max(1, std::max(2, 3, greater_than), greater_than);
194194
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use an initializer list instead [modernize-min-max-use-initializer-list]
195-
// CHECK-FIXES: int max6 = std::max({1, 2, 3}, greater_than);
195+
// CHECK-FIXES: int max6 = std::max({1, 2, 3 }, greater_than);
196196

197197
int min6 = std::min(1, std::min(2, 3, greater_than), greater_than);
198198
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use an initializer list instead [modernize-min-max-use-initializer-list]
199-
// CHECK-FIXES: int min6 = std::min({1, 2, 3}, greater_than);
199+
// CHECK-FIXES: int min6 = std::min({1, 2, 3 }, greater_than);
200200

201201
int max7 = std::max(1, std::max(2, 3, fless_than), fgreater_than);
202202
// CHECK-FIXES: int max7 = std::max(1, std::max(2, 3, fless_than), fgreater_than);
@@ -212,11 +212,11 @@ int min8 = std::min(1, std::min(2, 3, fless_than), less_than);
212212

213213
int max9 = std::max(1, std::max(2, 3, fless_than), fless_than);
214214
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::max' calls, use an initializer list instead [modernize-min-max-use-initializer-list]
215-
// CHECK-FIXES: int max9 = std::max({1, 2, 3}, fless_than);
215+
// CHECK-FIXES: int max9 = std::max({1, 2, 3 }, fless_than);
216216

217217
int min9 = std::min(1, std::min(2, 3, fless_than), fless_than);
218218
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not use nested 'std::min' calls, use an initializer list instead [modernize-min-max-use-initializer-list]
219-
// CHECK-FIXES: int min9 = std::min({1, 2, 3}, fless_than);
219+
// CHECK-FIXES: int min9 = std::min({1, 2, 3 }, fless_than);
220220

221221
int min10 = std::min(std::min(4, 5), std::max(2, utils::max(3, 1)));
222222
// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use nested 'std::min' calls, use an initializer list instead [modernize-min-max-use-initializer-list]
@@ -255,4 +255,14 @@ int macroVarMax2 = std::max(1, std::max<int>(value2, 2.0f));
255255
// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: do not use nested 'std::max' calls, use an initializer list instead [modernize-min-max-use-initializer-list]
256256
// CHECK-FIXES: int macroVarMax2 = std::max({1, static_cast<int>(value2), static_cast<int>(2.0f)});
257257

258+
// True-negative tests
259+
int maxTN1 = std::max(1, 2);
260+
// CHECK-FIXES: int maxTN1 = std::max(1, 2);
261+
262+
int maxTN2 = std::max({1, 2, 3});
263+
// CHECK-FIXES: int maxTN2 = std::max({1, 2, 3});
264+
265+
int maxTN3 = std::max({1, 2, 3}, less_than);
266+
// CHECK-FIXES: int maxTN3 = std::max({1, 2, 3}, less_than);
267+
258268
}

0 commit comments

Comments
 (0)