https://github.com/zeyi2 updated https://github.com/llvm/llvm-project/pull/169908
>From 3b7d92ba5fb8d5849e68369fc0f8ea56d1cf494d Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Fri, 28 Nov 2025 20:06:44 +0800 Subject: [PATCH 1/2] [clang-tidy] Preserve comments in `readability-use-std-min-max` --- .../readability/UseStdMinMaxCheck.cpp | 23 ++++++++++++------- clang-tools-extra/docs/ReleaseNotes.rst | 5 ++++ .../checkers/readability/use-std-min-max.cpp | 11 +++++++++ 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp index 5a7add88d6eeb..f78e62fbc3c27 100644 --- a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp @@ -96,12 +96,11 @@ static QualType getReplacementCastType(const Expr *CondLhs, const Expr *CondRhs, return GlobalImplicitCastType; } -static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs, - const Expr *AssignLhs, - const SourceManager &Source, - const LangOptions &LO, - StringRef FunctionName, - const BinaryOperator *BO) { +static std::string +createReplacement(const Expr *CondLhs, const Expr *CondRhs, + const Expr *AssignLhs, const SourceManager &Source, + const LangOptions &LO, StringRef FunctionName, + const BinaryOperator *BO, StringRef Comment = "") { const llvm::StringRef CondLhsStr = Lexer::getSourceText( Source.getExpansionRange(CondLhs->getSourceRange()), Source, LO); const llvm::StringRef CondRhsStr = Lexer::getSourceText( @@ -116,7 +115,7 @@ static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs, (!GlobalImplicitCastType.isNull() ? "<" + GlobalImplicitCastType.getAsString() + ">(" : "(") + - CondLhsStr + ", " + CondRhsStr + ");") + CondLhsStr + ", " + CondRhsStr + ");" + Comment) .str(); } @@ -172,13 +171,21 @@ void UseStdMinMaxCheck::check(const MatchFinder::MatchResult &Result) { auto ReplaceAndDiagnose = [&](const llvm::StringRef FunctionName) { const SourceManager &Source = *Result.SourceManager; + const std::string Comment = + Lexer::getSourceText( + CharSourceRange::getCharRange( + Lexer::getLocForEndOfToken(If->getRParenLoc(), 0, Source, LO), + If->getThen()->getBeginLoc()), + Source, LO) + .rtrim() + .str(); diag(IfLocation, "use `%0` instead of `%1`") << FunctionName << BinaryOp->getOpcodeStr() << FixItHint::CreateReplacement( SourceRange(IfLocation, Lexer::getLocForEndOfToken( ThenLocation, 0, Source, LO)), createReplacement(CondLhs, CondRhs, AssignLhs, Source, LO, - FunctionName, BinaryOp)) + FunctionName, BinaryOp, Comment)) << IncludeInserter.createIncludeInsertion( Source.getFileID(If->getBeginLoc()), AlgorithmHeader); }; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index a6f80e3721db1..d2da19b61132c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -555,6 +555,11 @@ Changes in existing checks <clang-tidy/checks/readability/use-concise-preprocessor-directives>` check to generate correct fix-its for forms without a space after the directive. +- Improved :doc:`readability-use-std-min-max + <clang-tidy/checks/readability/use-std-min-max>` check by ensuring that + comments between the ``if`` condition and the ``then`` block are preserved + when applying the fix. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp index 35ade8a7c6d37..f51bf1ae98ed1 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp @@ -273,3 +273,14 @@ void useRight() { } } // namespace gh121676 + +void testComment() { + int box_depth = 10; + int max_depth = 5; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // limit the depth to max + if (box_depth > max_depth) // limit the depth to max + { + box_depth = max_depth; + } +} >From 4674ddcc4d993fb677a1bf3d6ac51a9d738caf52 Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Wed, 3 Dec 2025 15:19:57 +0800 Subject: [PATCH 2/2] Fix --- .../readability/UseStdMinMaxCheck.cpp | 50 ++++- .../checkers/readability/use-std-min-max.cpp | 173 +++++++++++++----- 2 files changed, 170 insertions(+), 53 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp index f78e62fbc3c27..a7a439cf3e5bb 100644 --- a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp @@ -115,7 +115,8 @@ createReplacement(const Expr *CondLhs, const Expr *CondRhs, (!GlobalImplicitCastType.isNull() ? "<" + GlobalImplicitCastType.getAsString() + ">(" : "(") + - CondLhsStr + ", " + CondRhsStr + ");" + Comment) + CondLhsStr + ", " + CondRhsStr + ");" + (Comment.empty() ? "" : " ") + + Comment) .str(); } @@ -171,14 +172,45 @@ void UseStdMinMaxCheck::check(const MatchFinder::MatchResult &Result) { auto ReplaceAndDiagnose = [&](const llvm::StringRef FunctionName) { const SourceManager &Source = *Result.SourceManager; - const std::string Comment = - Lexer::getSourceText( - CharSourceRange::getCharRange( - Lexer::getLocForEndOfToken(If->getRParenLoc(), 0, Source, LO), - If->getThen()->getBeginLoc()), - Source, LO) - .rtrim() - .str(); + llvm::SmallString<64> Comment; + + const auto AppendNormalized = [&](llvm::StringRef Text) { + Text = Text.ltrim(); + if (!Text.empty()) { + if (!Comment.empty()) + Comment += " "; + Comment += Text; + } + }; + + AppendNormalized(Lexer::getSourceText( + CharSourceRange::getCharRange( + Lexer::getLocForEndOfToken(If->getRParenLoc(), 0, Source, LO), + If->getThen()->getBeginLoc()), + Source, LO)); + + if (const auto *CS = dyn_cast<CompoundStmt>(If->getThen())) { + const Stmt *Inner = CS->body_front(); + AppendNormalized(Lexer::getSourceText( + CharSourceRange::getCharRange( + Lexer::getLocForEndOfToken(CS->getBeginLoc(), 0, Source, LO), + Inner->getBeginLoc()), + Source, LO)); + + llvm::StringRef PostInner = Lexer::getSourceText( + CharSourceRange::getCharRange( + Lexer::getLocForEndOfToken(Inner->getEndLoc(), 0, Source, LO), + CS->getEndLoc()), + Source, LO); + + const size_t Semi = PostInner.find(';'); + if (Semi != llvm::StringRef::npos && + PostInner.take_front(Semi).trim().empty()) { + PostInner = PostInner.drop_front(Semi + 1); + } + AppendNormalized(PostInner); + } + diag(IfLocation, "use `%0` instead of `%1`") << FunctionName << BinaryOp->getOpcodeStr() << FixItHint::CreateReplacement( diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp index f51bf1ae98ed1..afeffb2604115 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp @@ -12,7 +12,7 @@ constexpr int myConstexprMax(int a, int b) { #define MY_IF_MACRO(condition, statement) \ if (condition) { \ statement \ - } + } class MyClass { public: @@ -28,22 +28,22 @@ void foo(T value7) { // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: value1 = std::max(value1, value2); if (value1 < value2) - value1 = value2; + value1 = value2; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: value2 = std::min(value1, value2); if (value1 < value2) - value2 = value1; + value2 = value1; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] // CHECK-FIXES: value2 = std::min(value2, value1); if (value2 > value1) - value2 = value1; + value2 = value1; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>` [readability-use-std-min-max] // CHECK-FIXES: value1 = std::max(value2, value1); if (value2 > value1) - value1 = value2; + value1 = value2; // No suggestion needed here if (value1 == value2) @@ -53,102 +53,102 @@ void foo(T value7) { // CHECK-FIXES: value1 = std::max<int>(value1, value4); short value4; if(value1<value4) - value1=value4; - + value1=value4; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: value3 = std::min(value1+value2, value3); if(value1+value2<value3) - value3 = value1+value2; - + value3 = value1+value2; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: value1 = std::max(value1, myConstexprMin(value2, value3)); if (value1 < myConstexprMin(value2, value3)) - value1 = myConstexprMin(value2, value3); - + value1 = myConstexprMin(value2, value3); + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] // CHECK-FIXES: value1 = std::min(value1, myConstexprMax(value2, value3)); if (value1 > myConstexprMax(value2, value3)) - value1 = myConstexprMax(value2, value3); - + value1 = myConstexprMax(value2, value3); + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<=` [readability-use-std-min-max] // CHECK-FIXES: value2 = std::min(value1, value2); if (value1 <= value2) - value2 = value1; + value2 = value1; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<=` [readability-use-std-min-max] // CHECK-FIXES: value1 = std::max(value1, value2); if (value1 <= value2) - value1 = value2; + value1 = value2; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>=` [readability-use-std-min-max] // CHECK-FIXES: value1 = std::max(value2, value1); if (value2 >= value1) - value1 = value2; + value1 = value2; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>=` [readability-use-std-min-max] // CHECK-FIXES: value2 = std::min(value2, value1); if (value2 >= value1) - value2 = value1; - + value2 = value1; + // CHECK-MESSAGES: :[[@LINE+3]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: obj.member1 = std::max(obj.member1, obj.member2); MyClass obj; if (obj.member1 < obj.member2) - obj.member1 = obj.member2; + obj.member1 = obj.member2; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: obj.member2 = std::min(obj.member1, obj.member2); if (obj.member1 < obj.member2) - obj.member2 = obj.member1; + obj.member2 = obj.member1; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] // CHECK-FIXES: obj.member2 = std::min(obj.member2, obj.member1); if (obj.member2 > obj.member1) - obj.member2 = obj.member1; + obj.member2 = obj.member1; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>` [readability-use-std-min-max] // CHECK-FIXES: obj.member1 = std::max(obj.member2, obj.member1); if (obj.member2 > obj.member1) - obj.member1 = obj.member2; - + obj.member1 = obj.member2; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: obj.member1 = std::max<int>(obj.member1, value4); if (obj.member1 < value4) - obj.member1 = value4; - + obj.member1 = value4; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: value3 = std::min(obj.member1 + value2, value3); if (obj.member1 + value2 < value3) - value3 = obj.member1 + value2; - + value3 = obj.member1 + value2; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `<=` [readability-use-std-min-max] // CHECK-FIXES: obj.member2 = std::min(value1, obj.member2); if (value1 <= obj.member2) - obj.member2 = value1; + obj.member2 = value1; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<=` [readability-use-std-min-max] // CHECK-FIXES: value1 = std::max(value1, obj.member2); if (value1 <= obj.member2) - value1 = obj.member2; + value1 = obj.member2; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `>=` [readability-use-std-min-max] // CHECK-FIXES: value1 = std::max(obj.member2, value1); if (obj.member2 >= value1) - value1 = obj.member2; + value1 = obj.member2; // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>=` [readability-use-std-min-max] // CHECK-FIXES: obj.member2 = std::min(obj.member2, value1); if (obj.member2 >= value1) - obj.member2 = value1; - + obj.member2 = value1; + // No suggestion needed here if (MY_MACRO_MIN(value1, value2) < value3) - value3 = MY_MACRO_MIN(value1, value2); - + value3 = MY_MACRO_MIN(value1, value2); + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::max` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: value4 = std::max<int>(value4, value2); if (value4 < value2){ - value4 = value2; + value4 = value2; } // No suggestion needed here @@ -156,18 +156,18 @@ void foo(T value7) { value2 = value1; else value2 = value3; - + // No suggestion needed here if(value1<value2){ - value2 = value1; + value2 = value1; } else{ - value2 = value3; + value2 = value3; } // No suggestion needed here if(value1<value2){ - value2 = value1; + value2 = value1; int res = value1 + value2; } @@ -202,7 +202,7 @@ void foo(T value7) { value2 = value3; } } - + // CHECK-MESSAGES: :[[@LINE+4]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] // CHECK-FIXES: value6 = std::min<unsigned int>(value5, value6); unsigned int value5; @@ -221,7 +221,7 @@ void foo(T value7) { const int value8 = 5; if(value8<value1) value1 = value8; - + //CHECK-MESSAGES: :[[@LINE+3]]:3: warning: use `std::min` instead of `<` [readability-use-std-min-max] //CHECK-FIXES: value1 = std::min(value9, value1); volatile int value9 = 6; @@ -274,13 +274,98 @@ void useRight() { } // namespace gh121676 -void testComment() { +void testComments() { int box_depth = 10; int max_depth = 5; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] - // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // limit the depth to max - if (box_depth > max_depth) // limit the depth to max + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here + if (box_depth > max_depth) // here { box_depth = max_depth; } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */ + if (box_depth > max_depth) /* here */ + { + box_depth = max_depth; + } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here + if (box_depth > max_depth) + // here + { + box_depth = max_depth; + } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */ + if (box_depth > max_depth) + /* here */ + { + box_depth = max_depth; + } + + // CHECK-MESSAGES: :[[@LINE+4]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here + // CHECK-FIXES-NEXT: and here + // CHECK-FIXES-NEXT: */ + if (box_depth > max_depth) /* here + and here + */ + { + box_depth = max_depth; + } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */ + if (box_depth > max_depth) { /* here */ + box_depth = max_depth; + } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here + if (box_depth > max_depth) { // here + box_depth = max_depth; + } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */ + if (box_depth > max_depth) { + box_depth = max_depth; /* here */ + } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here + if (box_depth > max_depth) { + box_depth = max_depth; // here + } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); /* here */ + if (box_depth > max_depth) { + box_depth = max_depth; + /* here */ + } + + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here + if (box_depth > max_depth) { + box_depth = max_depth; + // here + } + + // CHECK-MESSAGES: :[[@LINE+5]]:3: warning: use `std::min` instead of `>` [readability-use-std-min-max] + // CHECK-FIXES: box_depth = std::min(box_depth, max_depth); // here + // CHECK-FIXES-NEXT: // and + // CHECK-FIXES-NEXT: /* there + // CHECK-FIXES-NEXT: again*/ + if (box_depth > max_depth) { + // here + box_depth = max_depth; // and + /* there + again*/ + } } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
