Fix reported range of partial token replacement

[steveire]

Summary

Fixes bug: 38678

[steveire]

Fix reported range of partial token replacement

[steveire]

Move comment

[aaron.ballman]

I think the fix generally looks good, but can you please add some test coverage for the change?

[steveire]

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?

[aaron.ballman]

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

[steveire]

In D51192#1226282, @aaron.ballman wrote:

In 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?

[aaron.ballman]

In D51192#1226312, @steveire wrote:

In 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?

[steveire]

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?

[aaron.ballman]

In 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: <URL>

Where the URL is the URL to this review.

[steveire]

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?

[aaron.ballman]

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

clang-tidy/ClangTidy.cpp

SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
         Name += ",-warnings-as-errors";
         Level = DiagnosticsEngine::Error;
         WarningsAsErrors++;
       }
       auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
                   << Message.Message << Name;
       for (const auto &FileAndReplacements : Error.Fix) {
         for (const auto &Repl : 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;
           if (Repl.isApplicable()) {
             SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
             Files.makeAbsolutePath(FixAbsoluteFilePath);
             if (ApplyFixes) {
               tooling::Replacement R(FixAbsoluteFilePath, Repl.getOffset(),
SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
               } else {
                 CanBeApplied = true;
                 ++AppliedFixes;
               }
             }
             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());
           }
 
           if (ApplyFixes)
             FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
         }
       }