Page MenuHomePhabricator

[clang-tidy] Fix identifier naming in macro args.
ClosedPublic

Authored by jhen on Oct 10 2016, 1:44 PM.

Details

Summary

clang-tidy should fix identifier naming even when the identifier is
referenced inside a macro expansion, provided that the identifier enters
the macro expansion completely within a macro argument.

For example, this will allow fixes to the naming of the identifier
'global' when it is declared and used as follows:

int global;
#define USE_IN_MACRO(m) auto use_##m = m
USE_IN_MACRO(global);

Diff Detail

Repository
rL LLVM

Event Timeline

jhen updated this revision to Diff 74171.Oct 10 2016, 1:44 PM
jhen retitled this revision from to [clang-tidy] Fix identifier naming in macro args..
jhen updated this object.
jhen added a reviewer: alexfh.
jhen added a subscriber: cfe-commits.
jhen added a comment.Oct 10 2016, 1:45 PM

alexfh, sorry if you are not the right person to review this change. I based my choice on this history of this file.

jhen updated this revision to Diff 74181.Oct 10 2016, 3:25 PM
  • Return to original checking for macro in range
jhen added a comment.Oct 10 2016, 3:28 PM

I found a bug in my first patch that I have fixed now. I was trying to iterate over the source range by using SourceLocation::getLocWithOffset, but I realized that doesn't work, so I removed it and went back to the original method of checking SourceRange.getBegin().isMacroID() and SourceRange.getEnd().isMacroID().

jhen updated this revision to Diff 74184.Oct 10 2016, 4:28 PM
  • Prevent multiple fixes for macro expansion
jhen added a comment.Oct 10 2016, 4:30 PM

I just found and fixed another bug in this patch. Before, I wasn't using the spelling location for the fixit hint. This meant that a macro argument that was expanded to two locations, for example, would have the same fixit hint applied to it twice. My new test case verifies that this does not happen anymore.

Adding arron.ballman as a reviewer as alexfh seems to be on leave for a few weeks.

aaron.ballman added inline comments.Oct 20 2016, 6:13 AM
clang-tidy/readability/IdentifierNamingCheck.cpp
654 ↗(On Diff #74184)

We could do an early return if ShouldFix is already false and simplify this expression with ShouldFix = RangeCanBeFixed;, can't we?

jhen updated this revision to Diff 75430.Oct 21 2016, 8:01 AM
  • Early exit if not Failure.ShouldFix
jhen marked an inline comment as done.Oct 21 2016, 8:03 AM
jhen added inline comments.
clang-tidy/readability/IdentifierNamingCheck.cpp
654 ↗(On Diff #74184)

Thanks for pointing that out. The early exit is much more efficient.

aaron.ballman accepted this revision.Oct 21 2016, 8:25 AM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Oct 21 2016, 8:25 AM
This revision was automatically updated to reflect the committed changes.
jhen marked an inline comment as done.
jhen added a comment.Oct 24 2016, 10:30 AM

Thanks for the review!