[clang-tidy][clangd] Fixed removeFunctionArgs don't remove comma for use-ranges check #118568
Conversation
|
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 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. |
|
@llvm/pr-subscribers-clang-tools-extra Author: Richard Li (chomosuke) ChangesThe problem: The more general problem: Full diff: https://github.com/llvm/llvm-project/pull/118568.diff 1 Files Affected:
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()));
}
}
}
|
|
@llvm/pr-subscribers-clang-tidy Author: Richard Li (chomosuke) ChangesThe problem: The more general problem: Full diff: https://github.com/llvm/llvm-project/pull/118568.diff 1 Files Affected:
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()));
}
}
}
|
|
General cased fixed in #118569 |
|
Please mention changes in Release Notes. |
|
@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. |
There was a problem hiding this comment.
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. |
|
Ping~~~ |
|
Ping |
|
Ping |
| @@ -216,6 +219,9 @@ Changes in existing checks | |||
| <clang-tidy/checks/modernize/use-ranges>` check by updating suppress | |||
| warnings logic for ``nullptr`` in ``std::find``. | |||
|
|
|||
| - Improved :doc:`modernize-use-ranges | |||
There was a problem hiding this comment.
Please merge with existing entry.
| @@ -164,6 +165,24 @@ void UseRangesCheck::registerMatchers(MatchFinder *Finder) { | |||
| static void removeFunctionArgs(DiagnosticBuilder &Diag, const CallExpr &Call, | |||
| ArrayRef<unsigned> Indexes, | |||
| const ASTContext &Ctx) { | |||
| const auto GetCommaLoc = [&](SourceLocation BeginLoc, | |||
| SourceLocation EndLoc) -> CharSourceRange { | |||
There was a problem hiding this comment.
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.
| if (!NextTok) { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
| if (!NextTok) { | |
| return {}; | |
| } | |
| if (!NextTok) | |
| return {}; |
Don't use braces around single-statement if's as (LLVM coding guide)
| @@ -164,6 +165,24 @@ void UseRangesCheck::registerMatchers(MatchFinder *Finder) { | |||
| static void removeFunctionArgs(DiagnosticBuilder &Diag, const CallExpr &Call, | |||
| ArrayRef<unsigned> Indexes, | |||
| const ASTContext &Ctx) { | |||
| const auto GetCommaLoc = [&](SourceLocation BeginLoc, | |||
There was a problem hiding this comment.
Nit suggestion: as far as I can see, we only need Ctx here, so no point in capturing all variables.
| const auto GetCommaLoc = [&](SourceLocation BeginLoc, | |
| const auto GetCommaLoc = [&Ctx](SourceLocation BeginLoc, |
| @@ -148,6 +148,9 @@ New check aliases | |||
| Changes in existing checks | |||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ | |||
|
|
|||
| - Improved :doc:`boost-use-ranges | |||
| <clang-tidy/checks/boost/use-ranges>` check to more precisely remove comma. | |||
There was a problem hiding this comment.
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.
| <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.
The problem:
When running the code action for
mordernize-use-rangesonstd::reverse(v.begin(), v.end()), the code gets modified tostd::reverse(v,)instead ofstd::reverse(v). This PR fixes bothmordernize-use-rangesandboost-use-ranges.The more general problem:
This does not show up in tests for clang-tidy because
clang-tidy --fixrunsformat::cleanupAroundReplacements()before committing the fixes where as clangd does not. There are many other clang-tidy check that work withclangd-tidy --fixbut does not work with clangd code action. I will soon open an PR that fix this in the general case by runningformat::cleanupAroundReplacements()in clangd.