Skip to content

Commit 07662c9

Browse files
zeyi2Honey Goyal
zeyi2
authored and
Honey Goyal
committed
[clang-tidy] Preserve comments in readability-use-std-min-max (llvm#169908)
Closes [llvm#121613](llvm#121613)
1 parent 1eef550 commit 07662c9

File tree

3 files changed

+161
-8
lines changed

3 files changed

+161
-8
lines changed

‎clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp‎

Lines changed: 60 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -96,12 +96,11 @@ static QualType getReplacementCastType(const Expr *CondLhs, const Expr *CondRhs,
9696
return GlobalImplicitCastType;
9797
}
9898

99-
static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs,
100-
const Expr *AssignLhs,
101-
const SourceManager &Source,
102-
const LangOptions &LO,
103-
StringRef FunctionName,
104-
const BinaryOperator *BO) {
99+
static std::string
100+
createReplacement(const Expr *CondLhs, const Expr *CondRhs,
101+
const Expr *AssignLhs, const SourceManager &Source,
102+
const LangOptions &LO, StringRef FunctionName,
103+
const BinaryOperator *BO, StringRef Comment = "") {
105104
const llvm::StringRef CondLhsStr = Lexer::getSourceText(
106105
Source.getExpansionRange(CondLhs->getSourceRange()), Source, LO);
107106
const llvm::StringRef CondRhsStr = Lexer::getSourceText(
@@ -116,7 +115,8 @@ static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs,
116115
(!GlobalImplicitCastType.isNull()
117116
? "<" + GlobalImplicitCastType.getAsString() + ">("
118117
: "(") +
119-
CondLhsStr + ", " + CondRhsStr + ");")
118+
CondLhsStr + ", " + CondRhsStr + ");" + (Comment.empty() ? "" : " ") +
119+
Comment)
120120
.str();
121121
}
122122

@@ -172,13 +172,65 @@ void UseStdMinMaxCheck::check(const MatchFinder::MatchResult &Result) {
172172

173173
auto ReplaceAndDiagnose = [&](const llvm::StringRef FunctionName) {
174174
const SourceManager &Source = *Result.SourceManager;
175+
llvm::SmallString<64> Comment;
176+
177+
const auto AppendNormalized = [&](llvm::StringRef Text) {
178+
Text = Text.ltrim();
179+
if (!Text.empty()) {
180+
if (!Comment.empty())
181+
Comment += " ";
182+
Comment += Text;
183+
}
184+
};
185+
186+
const auto GetSourceText = [&](SourceLocation StartLoc,
187+
SourceLocation EndLoc) {
188+
return Lexer::getSourceText(
189+
CharSourceRange::getCharRange(
190+
Lexer::getLocForEndOfToken(StartLoc, 0, Source, LO), EndLoc),
191+
Source, LO);
192+
};
193+
194+
// Captures:
195+
// if (cond) // Comment A
196+
// if (cond) /* Comment A */ { ... }
197+
// if (cond) /* Comment A */ x = y;
198+
AppendNormalized(
199+
GetSourceText(If->getRParenLoc(), If->getThen()->getBeginLoc()));
200+
201+
if (const auto *CS = dyn_cast<CompoundStmt>(If->getThen())) {
202+
const Stmt *Inner = CS->body_front();
203+
204+
// Captures:
205+
// if (cond) { // Comment B
206+
// ...
207+
// }
208+
// if (cond) { /* Comment B */ x = y; }
209+
AppendNormalized(GetSourceText(CS->getBeginLoc(), Inner->getBeginLoc()));
210+
211+
// Captures:
212+
// if (cond) { x = y; // Comment C }
213+
// if (cond) { x = y; /* Comment C */ }
214+
llvm::StringRef PostInner =
215+
GetSourceText(Inner->getEndLoc(), CS->getEndLoc());
216+
217+
// Strip the trailing semicolon to avoid fixes like:
218+
// x = std::min(x, y);; // comment
219+
const size_t Semi = PostInner.find(';');
220+
if (Semi != llvm::StringRef::npos &&
221+
PostInner.take_front(Semi).trim().empty()) {
222+
PostInner = PostInner.drop_front(Semi + 1);
223+
}
224+
AppendNormalized(PostInner);
225+
}
226+
175227
diag(IfLocation, "use `%0` instead of `%1`")
176228
<< FunctionName << BinaryOp->getOpcodeStr()
177229
<< FixItHint::CreateReplacement(
178230
SourceRange(IfLocation, Lexer::getLocForEndOfToken(
179231
ThenLocation, 0, Source, LO)),
180232
createReplacement(CondLhs, CondRhs, AssignLhs, Source, LO,
181-
FunctionName, BinaryOp))
233+
FunctionName, BinaryOp, Comment))
182234
<< IncludeInserter.createIncludeInsertion(
183235
Source.getFileID(If->getBeginLoc()), AlgorithmHeader);
184236
};

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,11 @@ Changes in existing checks
579579
<clang-tidy/checks/readability/use-concise-preprocessor-directives>` check to
580580
generate correct fix-its for forms without a space after the directive.
581581

582+
- Improved :doc:`readability-use-std-min-max
583+
<clang-tidy/checks/readability/use-std-min-max>` check by ensuring that
584+
comments between the ``if`` condition and the ``then`` block are preserved
585+
when applying the fix.
586+
582587
Removed checks
583588
^^^^^^^^^^^^^^
584589

‎clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,3 +273,99 @@ void useRight() {
273273
}
274274

275275
} // namespace gh121676
276+
277+
void testComments() {
278+
int box_depth = 10;
279+
int max_depth = 5;
280+
281+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
282+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here
283+
if (box_depth > max_depth) // here
284+
{
285+
box_depth = max_depth;
286+
}
287+
288+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
289+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */
290+
if (box_depth > max_depth) /* here */
291+
{
292+
box_depth = max_depth;
293+
}
294+
295+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
296+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here
297+
if (box_depth > max_depth)
298+
// here
299+
{
300+
box_depth = max_depth;
301+
}
302+
303+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
304+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */
305+
if (box_depth > max_depth)
306+
/* here */
307+
{
308+
box_depth = max_depth;
309+
}
310+
311+
// CHECK-MESSAGES: :[[@LINE+4]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
312+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here
313+
// CHECK-FIXES-NEXT: and here
314+
// CHECK-FIXES-NEXT: */
315+
if (box_depth > max_depth) /* here
316+
and here
317+
*/
318+
{
319+
box_depth = max_depth;
320+
}
321+
322+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
323+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */
324+
if (box_depth > max_depth) { /* here */
325+
box_depth = max_depth;
326+
}
327+
328+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
329+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here
330+
if (box_depth > max_depth) { // here
331+
box_depth = max_depth;
332+
}
333+
334+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
335+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */
336+
if (box_depth > max_depth) {
337+
box_depth = max_depth; /* here */
338+
}
339+
340+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
341+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here
342+
if (box_depth > max_depth) {
343+
box_depth = max_depth; // here
344+
}
345+
346+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
347+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */
348+
if (box_depth > max_depth) {
349+
box_depth = max_depth;
350+
/* here */
351+
}
352+
353+
// CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
354+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here
355+
if (box_depth > max_depth) {
356+
box_depth = max_depth;
357+
// here
358+
}
359+
360+
// CHECK-MESSAGES: :[[@LINE+5]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max]
361+
// CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here
362+
// CHECK-FIXES-NEXT: // and
363+
// CHECK-FIXES-NEXT: /* there
364+
// CHECK-FIXES-NEXT: again*/
365+
if (box_depth > max_depth) {
366+
// here
367+
box_depth = max_depth; // and
368+
/* there
369+
again*/
370+
}
371+
}

0 commit comments

Comments
 (0)