[clang-tidy] Preserve comments in readability-use-std-min-max

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);
   };

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
 ^^^^^^^^^^^^^^
 

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*/
+  }
+}