Author: mitchell Date: 2025-12-05T09:41:31+08:00 New Revision: 9342d03f90ec42ca643f82a3e0654f71173a843f
URL: https://github.com/llvm/llvm-project/commit/9342d03f90ec42ca643f82a3e0654f71173a843f DIFF: https://github.com/llvm/llvm-project/commit/9342d03f90ec42ca643f82a3e0654f71173a843f.diff LOG: [clang-tidy] Preserve comments in `readability-use-std-min-max` (#169908) Closes [#121613](https://github.com/llvm/llvm-project/issues/121613) Added: Modified: clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp clang-tools-extra/docs/ReleaseNotes.rst clang-tools-extra/test/clang-tidy/checkers/readability/use-std-min-max.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseStdMinMaxCheck.cpp index 5a7add88d6eeb..ee0810b45559d 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,8 @@ static std::string createReplacement(const Expr *CondLhs, const Expr *CondRhs, (!GlobalImplicitCastType.isNull() ? "<" + GlobalImplicitCastType.getAsString() + ">(" : "(") + - CondLhsStr + ", " + CondRhsStr + ");") + CondLhsStr + ", " + CondRhsStr + ");" + (Comment.empty() ? "" : " ") + + Comment) .str(); } @@ -172,13 +172,65 @@ void UseStdMinMaxCheck::check(const MatchFinder::MatchResult &Result) { auto ReplaceAndDiagnose = [&](const llvm::StringRef FunctionName) { const SourceManager &Source = *Result.SourceManager; + llvm::SmallString<64> Comment; + + const auto AppendNormalized = [&](llvm::StringRef Text) { + Text = Text.ltrim(); + if (!Text.empty()) { + if (!Comment.empty()) + Comment += " "; + Comment += Text; + } + }; + + const auto GetSourceText = [&](SourceLocation StartLoc, + SourceLocation EndLoc) { + return Lexer::getSourceText( + CharSourceRange::getCharRange( + Lexer::getLocForEndOfToken(StartLoc, 0, Source, LO), EndLoc), + Source, LO); + }; + + // Captures: + // if (cond) // Comment A + // if (cond) /* Comment A */ { ... } + // if (cond) /* Comment A */ x = y; + AppendNormalized( + GetSourceText(If->getRParenLoc(), If->getThen()->getBeginLoc())); + + if (const auto *CS = dyn_cast<CompoundStmt>(If->getThen())) { + const Stmt *Inner = CS->body_front(); + + // Captures: + // if (cond) { // Comment B + // ... + // } + // if (cond) { /* Comment B */ x = y; } + AppendNormalized(GetSourceText(CS->getBeginLoc(), Inner->getBeginLoc())); + + // Captures: + // if (cond) { x = y; // Comment C } + // if (cond) { x = y; /* Comment C */ } + llvm::StringRef PostInner = + GetSourceText(Inner->getEndLoc(), CS->getEndLoc()); + + // Strip the trailing semicolon to avoid fixes like: + // x = std::min(x, y);; // comment + 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( 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 fdc866d8255bb..1e57ac1aa455c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -579,6 +579,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..35570189e1122 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,99 @@ void useRight() { } } // namespace gh121676 + +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); // 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
