[PATCH] D51192: Fix reported range of partial token replacement
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE341583: Fix reported range of partial token replacement
(authored by steveire, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D51192?vs=163447=164275#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51192
Files:
clang-tidy/ClangTidy.cpp
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -122,10 +122,6 @@
<< Message.Message << Name;
for (const auto : Error.Fix) {
for (const auto : FileAndReplacements.second) {
- // Retrieve the source range for applicable fixes. Macro definitions
- // on the command line have locations in a virtual buffer and don't
- // have valid file paths and are therefore not applicable.
- SourceRange Range;
SourceLocation FixLoc;
++TotalFixes;
bool CanBeApplied = false;
@@ -166,7 +162,11 @@
FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
SourceLocation FixEndLoc =
FixLoc.getLocWithOffset(Repl.getLength());
-Range = SourceRange(FixLoc, FixEndLoc);
+// Retrieve the source range for applicable fixes. Macro
definitions
+// on the command line have locations in a virtual buffer and don't
+// have valid file paths and are therefore not applicable.
+CharSourceRange Range =
+CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc));
Diag << FixItHint::CreateReplacement(Range,
Repl.getReplacementText());
}
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -122,10 +122,6 @@
<< Message.Message << Name;
for (const auto : Error.Fix) {
for (const auto : FileAndReplacements.second) {
- // Retrieve the source range for applicable fixes. Macro definitions
- // on the command line have locations in a virtual buffer and don't
- // have valid file paths and are therefore not applicable.
- SourceRange Range;
SourceLocation FixLoc;
++TotalFixes;
bool CanBeApplied = false;
@@ -166,7 +162,11 @@
FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
SourceLocation FixEndLoc =
FixLoc.getLocWithOffset(Repl.getLength());
-Range = SourceRange(FixLoc, FixEndLoc);
+// Retrieve the source range for applicable fixes. Macro definitions
+// on the command line have locations in a virtual buffer and don't
+// have valid file paths and are therefore not applicable.
+CharSourceRange Range =
+CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc));
Diag << FixItHint::CreateReplacement(Range,
Repl.getReplacementText());
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
aaron.ballman added a comment. In https://reviews.llvm.org/D51192#1226341, @steveire wrote: > Thanks. > > The `arc` tool already inserted `Differential Revision:` into my git commit, > but that's not what I wonder about. I'm looking for something I can insert > into my git commit so that the bug will automatically be closed when I commit > (and so that the bug will get a link to this PR when the PR is created). Does > such a thing exist here? Ah, sorry for the misunderstanding. I don't think such a thing exists here, unfortunately. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
steveire added a comment. Thanks. The `arc` tool already inserted `Differential Revision:` into my git commit, but that's not what I wonder about. I'm looking for something I can insert into my git commit so that the bug will automatically be closed when I commit (and so that the bug will get a link to this PR when the PR is created). Does such a thing exist here? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D51192#1226326, @steveire wrote: > As far as I know, no existing clang-tidy checks are affected. I discovered > this while implementing a custom check. See > https://bugs.llvm.org/show_bug.cgi?id=38678 It's unfortunate this is going in without tests, but it still LGTM. > By the way, is there some keyword I should use in commit messages to link to > bugs properly? Add the following to the end of the commit message and I believe it will automatically close the Phab review: Differential Revision: Where the URL is the URL to this review. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
steveire added a comment. As far as I know, no existing clang-tidy checks are affected. I discovered this while implementing a custom check. See https://bugs.llvm.org/show_bug.cgi?id=38678 By the way, is there some keyword I should use in commit messages to link to bugs properly? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
aaron.ballman added a comment. In https://reviews.llvm.org/D51192#1226312, @steveire wrote: > In https://reviews.llvm.org/D51192#1226282, @aaron.ballman wrote: > > > I'd probably pipe the diagnostic output to a temporary file that gets > > FileChecked with strict whitespace to see if the underlines from the > > diagnostic match the expected locations. We do this in a few Clang tests, > > like SemaCXX\struct-class-redecl.cpp or Misc\wrong-encoding.c. > > > Doesn't this require building-in a new check to clang-tidy which exists only > for the purpose of the test? Otherwise how would a test similar to > `SemaCXX\struct-class-redecl.cpp` work? What would be in the `RUN` line? Oh, I had the impression that this was changing the behavior of existing checks in the tree as well, and that we'd have an existing clang-tidy check that exhibits the problematic behavior. Is that not the case? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
steveire added a comment. In https://reviews.llvm.org/D51192#1226282, @aaron.ballman wrote: > In https://reviews.llvm.org/D51192#1226257, @steveire wrote: > > > How? This is 'private' code. I don't think it's worth changing that or > > creating a test with a huge amount of infrastructure in order to test this > > indirectly. > > > This is changing the observable behavior of the tool, so it should have tests > unless they're impossible to write. Yes. The current behavior is not tested. I agree that tests are better than no tests. >> Am I missing something? > > I'd probably pipe the diagnostic output to a temporary file that gets > FileChecked with strict whitespace to see if the underlines from the > diagnostic match the expected locations. We do this in a few Clang tests, > like SemaCXX\struct-class-redecl.cpp or Misc\wrong-encoding.c. Doesn't this require building-in a new check to clang-tidy which exists only for the purpose of the test? Otherwise how would a test similar to `SemaCXX\struct-class-redecl.cpp` work? What would be in the `RUN` line? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
aaron.ballman added a comment. In https://reviews.llvm.org/D51192#1226257, @steveire wrote: > How? This is 'private' code. I don't think it's worth changing that or > creating a test with a huge amount of infrastructure in order to test this > indirectly. This is changing the observable behavior of the tool, so it should have tests unless they're impossible to write. > Am I missing something? I'd probably pipe the diagnostic output to a temporary file that gets FileChecked with strict whitespace to see if the underlines from the diagnostic match the expected locations. We do this in a few Clang tests, like SemaCXX\struct-class-redecl.cpp or Misc\wrong-encoding.c. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
steveire added a comment. How? This is 'private' code. I don't think it's worth changing that or creating a test with a huge amount of infrastructure in order to test this indirectly. Am I missing something? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
aaron.ballman added a comment. I think the fix generally looks good, but can you please add some test coverage for the change? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
steveire updated this revision to Diff 163447.
steveire added a comment.
Move comment
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51192
Files:
clang-tidy/ClangTidy.cpp
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -122,10 +122,6 @@
<< Message.Message << Name;
for (const auto : Error.Fix) {
for (const auto : FileAndReplacements.second) {
- // Retrieve the source range for applicable fixes. Macro definitions
- // on the command line have locations in a virtual buffer and don't
- // have valid file paths and are therefore not applicable.
- SourceRange Range;
SourceLocation FixLoc;
++TotalFixes;
bool CanBeApplied = false;
@@ -166,7 +162,11 @@
FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
SourceLocation FixEndLoc =
FixLoc.getLocWithOffset(Repl.getLength());
-Range = SourceRange(FixLoc, FixEndLoc);
+// Retrieve the source range for applicable fixes. Macro
definitions
+// on the command line have locations in a virtual buffer and don't
+// have valid file paths and are therefore not applicable.
+CharSourceRange Range =
+CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc));
Diag << FixItHint::CreateReplacement(Range,
Repl.getReplacementText());
}
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -122,10 +122,6 @@
<< Message.Message << Name;
for (const auto : Error.Fix) {
for (const auto : FileAndReplacements.second) {
- // Retrieve the source range for applicable fixes. Macro definitions
- // on the command line have locations in a virtual buffer and don't
- // have valid file paths and are therefore not applicable.
- SourceRange Range;
SourceLocation FixLoc;
++TotalFixes;
bool CanBeApplied = false;
@@ -166,7 +162,11 @@
FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
SourceLocation FixEndLoc =
FixLoc.getLocWithOffset(Repl.getLength());
-Range = SourceRange(FixLoc, FixEndLoc);
+// Retrieve the source range for applicable fixes. Macro definitions
+// on the command line have locations in a virtual buffer and don't
+// have valid file paths and are therefore not applicable.
+CharSourceRange Range =
+CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc));
Diag << FixItHint::CreateReplacement(Range,
Repl.getReplacementText());
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
steveire updated this revision to Diff 162288. steveire added a comment. Fix reported range of partial token replacement Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 Files: clang-tidy/ClangTidy.cpp Index: clang-tidy/ClangTidy.cpp === --- clang-tidy/ClangTidy.cpp +++ clang-tidy/ClangTidy.cpp @@ -125,7 +125,6 @@ // Retrieve the source range for applicable fixes. Macro definitions // on the command line have locations in a virtual buffer and don't // have valid file paths and are therefore not applicable. - SourceRange Range; SourceLocation FixLoc; ++TotalFixes; bool CanBeApplied = false; @@ -166,7 +165,7 @@ FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset()); SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Repl.getLength()); -Range = SourceRange(FixLoc, FixEndLoc); +CharSourceRange Range = CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc)); Diag << FixItHint::CreateReplacement(Range, Repl.getReplacementText()); } Index: clang-tidy/ClangTidy.cpp === --- clang-tidy/ClangTidy.cpp +++ clang-tidy/ClangTidy.cpp @@ -125,7 +125,6 @@ // Retrieve the source range for applicable fixes. Macro definitions // on the command line have locations in a virtual buffer and don't // have valid file paths and are therefore not applicable. - SourceRange Range; SourceLocation FixLoc; ++TotalFixes; bool CanBeApplied = false; @@ -166,7 +165,7 @@ FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset()); SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Repl.getLength()); -Range = SourceRange(FixLoc, FixEndLoc); +CharSourceRange Range = CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc)); Diag << FixItHint::CreateReplacement(Range, Repl.getReplacementText()); } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51192: Fix reported range of partial token replacement
steveire created this revision. steveire added reviewers: klimek, rsmith. Herald added a subscriber: cfe-commits. Fixes bug: 38678 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51192 Files: clang-tidy/ClangTidy.cpp Index: clang-tidy/ClangTidy.cpp === --- clang-tidy/ClangTidy.cpp +++ clang-tidy/ClangTidy.cpp @@ -125,7 +125,6 @@ // Retrieve the source range for applicable fixes. Macro definitions // on the command line have locations in a virtual buffer and don't // have valid file paths and are therefore not applicable. - SourceRange Range; SourceLocation FixLoc; ++TotalFixes; bool CanBeApplied = false; @@ -166,7 +165,7 @@ FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset()); SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Repl.getLength()); -Range = SourceRange(FixLoc, FixEndLoc); +CharSourceRange Range = CharSourceRange::getCharRange(FixLoc, FixEndLoc); Diag << FixItHint::CreateReplacement(Range, Repl.getReplacementText()); } Index: clang-tidy/ClangTidy.cpp === --- clang-tidy/ClangTidy.cpp +++ clang-tidy/ClangTidy.cpp @@ -125,7 +125,6 @@ // Retrieve the source range for applicable fixes. Macro definitions // on the command line have locations in a virtual buffer and don't // have valid file paths and are therefore not applicable. - SourceRange Range; SourceLocation FixLoc; ++TotalFixes; bool CanBeApplied = false; @@ -166,7 +165,7 @@ FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset()); SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Repl.getLength()); -Range = SourceRange(FixLoc, FixEndLoc); +CharSourceRange Range = CharSourceRange::getCharRange(FixLoc, FixEndLoc); Diag << FixItHint::CreateReplacement(Range, Repl.getReplacementText()); } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
