[clang-tidy][clangd] Fixed removeFunctionArgs don't remove comma for use-ranges check

[chomosuke]

The problem:
When running the code action for mordernize-use-ranges on std::reverse(v.begin(), v.end()), the code gets modified to std::reverse(v,) instead of std::reverse(v). This PR fixes both mordernize-use-ranges and boost-use-ranges.

The more general problem:
This does not show up in tests for clang-tidy because clang-tidy --fix runs format::cleanupAroundReplacements() before committing the fixes where as clangd does not. There are many other clang-tidy check that work with clant-tidy --fix but does not work with clangd code action. I will soon open an PR that fix this in the general case by running format::cleanupAroundReplacements() in clangd.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: Richard Li (chomosuke)

Changes

The problem:
When running the code action for mordernize-use-ranges on std::reverse(v.begin(), v.end()), the code gets modified to std::reverse(v,) instead of std::reverse(v). This PR fixes both mordernize-use-ranges and boost-use-ranges.

The more general problem:
This does not show up in tests for clang-tidy because clang-tidy --fix runs format::cleanupAroundReplacements() before committing the fixes where as clangd does not. There are many other clang-tidy check that work with clant-tidy --fix but does not work with clangd code action. I will soon open an PR that fix this in the general case by running format::cleanupAroundReplacements() in clangd.

---

Full diff: https://github.com/llvm/llvm-project/pull/118568.diff

1 Files Affected:

• (modified) clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp (+10-9)

diff --git a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
index aba4d17ccd035e..88cba70b931d5d 100644
--- a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
@@ -28,6 +28,7 @@
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
+#include <iostream>
 #include <optional>
 #include <string>
 
@@ -173,21 +174,21 @@ static void removeFunctionArgs(DiagnosticBuilder &Diag, const CallExpr &Call,
   for (unsigned Index : Sorted) {
     const Expr *Arg = Call.getArg(Index);
     if (Commas[Index]) {
-      if (Index >= Commas.size()) {
-        Diag << FixItHint::CreateRemoval(Arg->getSourceRange());
-      } else {
+      if (Index + 1 < Call.getNumArgs()) {
         // Remove the next comma
         Commas[Index + 1] = true;
+        const Expr *NextArg = Call.getArg(Index + 1);
         Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
-            {Arg->getBeginLoc(),
-             Lexer::getLocForEndOfToken(
-                 Arg->getEndLoc(), 0, Ctx.getSourceManager(), Ctx.getLangOpts())
-                 .getLocWithOffset(1)}));
+            {Arg->getBeginLoc(), NextArg->getBeginLoc().getLocWithOffset(-1)}));
+      } else {
+        Diag << FixItHint::CreateRemoval(Arg->getSourceRange());
       }
     } else {
-      Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
-          Arg->getBeginLoc().getLocWithOffset(-1), Arg->getEndLoc()));
+      // At this point we know Index > 0 because `Commas[0] = true` earlier
       Commas[Index] = true;
+      const Expr *PrevArg = Call.getArg(Index - 1);
+      Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+          PrevArg->getEndLoc().getLocWithOffset(1), Arg->getEndLoc()));
     }
   }
 }

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Richard Li (chomosuke)

Changes

The problem:
When running the code action for mordernize-use-ranges on std::reverse(v.begin(), v.end()), the code gets modified to std::reverse(v,) instead of std::reverse(v). This PR fixes both mordernize-use-ranges and boost-use-ranges.

The more general problem:
This does not show up in tests for clang-tidy because clang-tidy --fix runs format::cleanupAroundReplacements() before committing the fixes where as clangd does not. There are many other clang-tidy check that work with clant-tidy --fix but does not work with clangd code action. I will soon open an PR that fix this in the general case by running format::cleanupAroundReplacements() in clangd.

---

Full diff: https://github.com/llvm/llvm-project/pull/118568.diff

1 Files Affected:

• (modified) clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp (+10-9)

diff --git a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
index aba4d17ccd035e..88cba70b931d5d 100644
--- a/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp
@@ -28,6 +28,7 @@
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
+#include <iostream>
 #include <optional>
 #include <string>
 
@@ -173,21 +174,21 @@ static void removeFunctionArgs(DiagnosticBuilder &Diag, const CallExpr &Call,
   for (unsigned Index : Sorted) {
     const Expr *Arg = Call.getArg(Index);
     if (Commas[Index]) {
-      if (Index >= Commas.size()) {
-        Diag << FixItHint::CreateRemoval(Arg->getSourceRange());
-      } else {
+      if (Index + 1 < Call.getNumArgs()) {
         // Remove the next comma
         Commas[Index + 1] = true;
+        const Expr *NextArg = Call.getArg(Index + 1);
         Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
-            {Arg->getBeginLoc(),
-             Lexer::getLocForEndOfToken(
-                 Arg->getEndLoc(), 0, Ctx.getSourceManager(), Ctx.getLangOpts())
-                 .getLocWithOffset(1)}));
+            {Arg->getBeginLoc(), NextArg->getBeginLoc().getLocWithOffset(-1)}));
+      } else {
+        Diag << FixItHint::CreateRemoval(Arg->getSourceRange());
       }
     } else {
-      Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
-          Arg->getBeginLoc().getLocWithOffset(-1), Arg->getEndLoc()));
+      // At this point we know Index > 0 because `Commas[0] = true` earlier
       Commas[Index] = true;
+      const Expr *PrevArg = Call.getArg(Index - 1);
+      Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+          PrevArg->getEndLoc().getLocWithOffset(1), Arg->getEndLoc()));
     }
   }
 }

[chomosuke]

General cased fixed in #118569

[EugeneZelenko]

Please mention changes in Release Notes.

[chomosuke]

@EugeneZelenko Mentioned in ReleaseNotes in #118569, since the ReleaseNotes of #118569 would include the changes in this PR. Please let me know if more details are required in the ReleaseNotes.