This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Skip reporting of not applicable fixes.
ClosedPublic

Authored by etienneb on Mar 17 2016, 8:25 PM.

Details

Summary

Invalid source location are causing clang-tidy to crash when manipulating an invalid file.

Macro definitions on the command line have locations in a virtual buffer and therefore
don't have a corresponding valid FilePath.

A recent patch added path conversion to absolute path. As the FilePath may now be empty,
the result of makeAbsolutePath may incorrectly be the folder WorkingDir. The crash occurs
in getLocation which is not able to find the appropriate FileEntry (null pointer).

SmallString<128> FixAbsoluteFilePath = Fix.getFilePath();
Files.makeAbsolutePath(FixAbsoluteFilePath);
FixLoc = getLocation(FixAbsoluteFilePath, Fix.getOffset());

With relative path, the code was not crashing because getLocation was skipping empty path.

Example of code:

int main() { return X; }

With the given command-line:

clang-tidy test.cc --checks=misc-macro-*  --  -DX=0+0

Diff Detail

Event Timeline

etienneb updated this revision to Diff 51010.Mar 17 2016, 8:25 PM
etienneb retitled this revision from to [clang-tidy] Skip reporting of not applicable fixes..
etienneb updated this object.
etienneb added reviewers: rnk, emso, alexfh, bkramer.
etienneb added a subscriber: cfe-commits.

Thank you for working on this! A question below, but also, the patch is missing test cases for the change.

ClangTidy.cpp
144

Is there a purpose to emitting the CreateReplacement if the fix isn't applicable?

alexfh edited edge metadata.Mar 18 2016, 7:17 AM

Thank you, Etienne!

Please add a regression test.

ClangTidy.cpp
144

No, I don't think we need a replacement with an invalid range.

alexfh edited reviewers, added: aaron.ballman; removed: bkramer, emso, rnk.Mar 18 2016, 7:17 AM
etienneb updated this revision to Diff 51046.Mar 18 2016, 11:20 AM
etienneb marked 2 inline comments as done.

Added test.

ClangTidy.cpp
144

The CreateReplacement fix is ignored when appended. In the append function, there is a check to avoid invalid source range.

But, I think it's cleaner to move the statement within the if-block.

aaron.ballman accepted this revision.Mar 22 2016, 6:45 AM
aaron.ballman edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Mar 22 2016, 6:45 AM
alexfh accepted this revision.Mar 22 2016, 7:32 AM
alexfh edited edge metadata.

LG. Thanks for the fix!

etienneb updated this revision to Diff 51305.Mar 22 2016, 10:55 AM
etienneb edited edge metadata.

rebase + fixing invalid paths

etienneb closed this revision.Mar 22 2016, 10:56 AM