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

chomosuke · 2024-12-04T00:47:05Z

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 clangd-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 · 2024-12-04T00:47:25Z

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 · 2024-12-04T00:47:58Z

@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 · 2024-12-04T00:47:59Z

@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 · 2024-12-04T14:30:25Z

General cased fixed in #118569

EugeneZelenko · 2024-12-04T15:11:31Z

Please mention changes in Release Notes.

chomosuke · 2024-12-04T20:25:46Z

@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.

5chmidti

No release note is fine for this patch, as this fixes a problem that is invisible when using clang-tidy, but visible with clangd. The general case for clangd is fixed in #118569. Otherwise, I don't think this would be a release note for clang-tidy, but for clangd (IMO). WDYT @EugeneZelenko

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp

EugeneZelenko · 2024-12-19T23:19:45Z

No release note is fine for this patch, as this fixes a problem that is invisible when using clang-tidy, but visible with clangd. The general case for clangd is fixed in #118569. Otherwise, I don't think this would be a release note for clang-tidy, but for clangd (IMO). WDYT @EugeneZelenko

I agree with that.

EugeneZelenko

Please mention changes in Release Notes.

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp

chomosuke · 2025-02-10T14:24:06Z

Ping~~~

chomosuke · 2025-03-22T14:21:52Z

Ping

chomosuke · 2025-05-24T19:45:51Z

Ping

EugeneZelenko · 2025-05-25T03:04:10Z

clang-tools-extra/docs/ReleaseNotes.rst

  <clang-tidy/checks/modernize/use-ranges>` check by updating suppress
  warnings logic for ``nullptr`` in ``std::find``.

+- Improved :doc:`modernize-use-ranges


Please merge with existing entry.

vbvictor · 2025-05-25T04:51:19Z

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp

                               ArrayRef<unsigned> Indexes,
                               const ASTContext &Ctx) {
+  const auto GetCommaLoc = [&](SourceLocation BeginLoc,
+                         SourceLocation EndLoc) -> CharSourceRange {


This formatting look odd, could you please run git clang-format on your changes.
Usually there is a job called "Check code formatting / code_formatter (pull_request)" that report such issues, but somehow you don't have it in your PR.

vbvictor · 2025-05-25T04:54:12Z

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp

+    if (!NextTok) {
+      return {};
+    }


Suggested change

if (!NextTok) {

return {};

}

if (!NextTok)

return {};

Don't use braces around single-statement if's as (LLVM coding guide)

vbvictor · 2025-05-25T05:00:12Z

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp

 static void removeFunctionArgs(DiagnosticBuilder &Diag, const CallExpr &Call,
                               ArrayRef<unsigned> Indexes,
                               const ASTContext &Ctx) {
+  const auto GetCommaLoc = [&](SourceLocation BeginLoc,


Nit suggestion: as far as I can see, we only need Ctx here, so no point in capturing all variables.

Suggested change

const auto GetCommaLoc = [&](SourceLocation BeginLoc,

const auto GetCommaLoc = [&Ctx](SourceLocation BeginLoc,

vbvictor · 2025-05-25T05:08:21Z

clang-tools-extra/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^^^^^^

+- Improved :doc:`boost-use-ranges
+  <clang-tidy/checks/boost/use-ranges>` check to more precisely remove comma.


In discussion, I saw that we don't need release notes for this change.
However, if there is a note, then it should also state that this change affects clangd.

Suggested change

<clang-tidy/checks/boost/use-ranges>` check to more precisely remove comma.

<clang-tidy/checks/boost/use-ranges>` check to more precisely remove comma when creating fix-its with :program:`clangd` code action.

Note that lines should be no more than 80 characters long.

fixed removeFunctionArgs don't remove comma

b43a260

llvmbot added clang-tools-extra clang-tidy labels Dec 4, 2024

Merge branch 'main' into fix-leave-comma

8a3f82f

Merge branch 'main' into fix-leave-comma

61ea649

chomosuke mentioned this pull request Dec 4, 2024

[clangd] Make clangd run format::cleanupAroundReplacements() for all code actions just as clang-tidy does #118569

Open

EugeneZelenko requested review from 5chmidti, HerrCai0907 and PiotrZSL December 4, 2024 15:10

5chmidti reviewed Dec 19, 2024

View reviewed changes

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

chomosuke added 2 commits December 25, 2024 22:11

Merge branch 'main' into fix-leave-comma

8082e34

find , and remove only ,

644c849

chomosuke requested a review from 5chmidti December 25, 2024 11:17

EugeneZelenko reviewed Dec 25, 2024

View reviewed changes

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

Merge branch 'main' into fix-leave-comma

2af7804

HerrCai0907 reviewed Dec 27, 2024

View reviewed changes

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

5chmidti reviewed Dec 28, 2024

View reviewed changes

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

chomosuke added 2 commits January 2, 2025 20:19

no auto when type isn't obvious and added to release note

2e80b9b

Merge branch 'main' into fix-leave-comma

4c75da3

PiotrZSL approved these changes Jan 13, 2025

View reviewed changes

chomosuke added 2 commits January 14, 2025 12:02

Merge branch 'main' into fix-leave-comma

e3d3ad0

Addressed comments

323fbe3

chomosuke requested a review from HerrCai0907 January 15, 2025 08:33

chomosuke requested review from 5chmidti, EugeneZelenko and PiotrZSL January 15, 2025 08:33

Merge branch 'main' into fix-leave-comma

2d49638

EugeneZelenko reviewed Jan 15, 2025

View reviewed changes

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/utils/UseRangesCheck.cpp Outdated Show resolved Hide resolved

chomosuke added 2 commits January 29, 2025 20:35

Merge remote-tracking branch 'upstream/main' into fix-leave-comma

2b40684

addressed const comments

1062399

chomosuke requested a review from EugeneZelenko January 30, 2025 06:01

Merge branch 'main' into fix-leave-comma

bcf5de1

chomosuke added 2 commits February 11, 2025 17:39

Merge branch 'main' into fix-leave-comma

84cf8d0

Merge branch 'main' into fix-leave-comma

9c42c9f

Merge branch 'main' into fix-leave-comma

19b69fc

EugeneZelenko requested a review from vbvictor May 24, 2025 23:55

Merge branch 'main' into fix-leave-comma

56d8cee

EugeneZelenko reviewed May 25, 2025

View reviewed changes

vbvictor requested changes May 25, 2025

View reviewed changes

	const auto GetCommaLoc = [&](SourceLocation BeginLoc,
	const auto GetCommaLoc = [&Ctx](SourceLocation BeginLoc,

	<clang-tidy/checks/boost/use-ranges>` check to more precisely remove comma.
	<clang-tidy/checks/boost/use-ranges>` check to more precisely remove comma when creating fix-its with :program:`clangd` code action.

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

chomosuke commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 4, 2024

Uh oh!

llvmbot commented Dec 4, 2024

Uh oh!

llvmbot commented Dec 4, 2024

Uh oh!

chomosuke commented Dec 4, 2024

Uh oh!

EugeneZelenko commented Dec 4, 2024

Uh oh!

chomosuke commented Dec 4, 2024

Uh oh!

5chmidti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EugeneZelenko commented Dec 19, 2024

Uh oh!

EugeneZelenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chomosuke commented Feb 10, 2025

Uh oh!

chomosuke commented Mar 22, 2025

Uh oh!

chomosuke commented May 24, 2025

Uh oh!

EugeneZelenko May 25, 2025

Choose a reason for hiding this comment

Uh oh!

vbvictor May 25, 2025

Choose a reason for hiding this comment

Uh oh!

vbvictor May 25, 2025

Choose a reason for hiding this comment

Uh oh!

vbvictor May 25, 2025

Choose a reason for hiding this comment

Uh oh!

vbvictor May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

chomosuke commented Dec 4, 2024 •

edited

Loading

vbvictor May 25, 2025 •

edited

Loading