Page MenuHomePhabricator

[clang-tidy] readability-identifier-naming - Support for Macros
ClosedPublic

Authored by JamesReynolds on Jun 6 2016, 7:24 AM.

Details

Summary

Added support for macro definitions.

  1. Added a pre-processor callback to catch macro definitions
  2. Changed the type of the failure map so that macros and declarations can share the same map
  3. Added extra tests to ensure fix-ups work using the new map
  4. Added fix-ups for type aliases in variable and function declarations as part of adding the new tests

Diff Detail

Repository
rL LLVM

Event Timeline

JamesReynolds retitled this revision from to [clang-tidy] readability-identifier-naming - Support for Macros.
JamesReynolds updated this object.
JamesReynolds added a reviewer: alexfh.
JamesReynolds added a subscriber: cfe-commits.

It'll be good idea to mention improvement in docs/ReleaseNotes.rst.

Please close PR23198 after commit.

Thanks Eugene, I've added a comment into docs/ReleaseNotes.rst to say this is in. I'll create a BugZilla account to update PR.

I've also added a new test and code to create FixIts for uses of the Macros as well as the definitions themselves.

Eugene.Zelenko added inline comments.Jun 7 2016, 11:51 AM
docs/ReleaseNotes.rst
262 ↗(On Diff #59857)

Please put this not in alphabetical order. I just fixed misleading order of misc-unconventional-assign-operator.

Changed the clang-tidy release notes addition to be in alphabetical order.

JamesReynolds marked an inline comment as done.Jun 8 2016, 1:39 AM
alexfh added inline comments.Jun 8 2016, 8:30 AM
clang-tidy/readability/IdentifierNamingCheck.cpp
142 ↗(On Diff #60010)

I'm not sure the check currently refuses to fix stuff defined in headers. It probably relies on -header-filter to limit its scope. It's intrinsically dangerous (since we should see all translation units using the entity to correctly rename it), but we should let users do this, in case they know what to do.

152 ↗(On Diff #60010)

No need to mute -Wunused _this_ way. Just comment out parameter names in the declaration.

814 ↗(On Diff #60010)

Remove = SourceRange. Just SourceRange Range(...); is enough.

clang-tidy/readability/IdentifierNamingCheck.h
86 ↗(On Diff #59715)

Maybe just typedef?

88 ↗(On Diff #59715)

Delegating constructors don't work in VS2013, which LLVM should still support.

JamesReynolds marked 5 inline comments as done.

Applied suggested code changes.

JamesReynolds marked an inline comment as not done.

Missed one modification in last update.

JamesReynolds marked an inline comment as done.Jun 8 2016, 10:02 AM
JamesReynolds added inline comments.Jun 8 2016, 10:03 AM
clang-tidy/readability/IdentifierNamingCheck.h
89 ↗(On Diff #60058)

I think I thought that this wouldn't get picked up by the specialization machinery - but you're right, a typedef works out fine! Maybe I was thinking of overloads...

JamesReynolds marked an inline comment as done.Jun 8 2016, 10:03 AM
alexfh accepted this revision.Jun 16 2016, 9:39 AM
alexfh edited edge metadata.

Looks good with a couple of nits.

clang-tidy/readability/IdentifierNamingCheck.cpp
808 ↗(On Diff #60058)

return?

clang-tidy/readability/IdentifierNamingCheck.h
94 ↗(On Diff #60058)

Please add trailing periods to comments. See also http://llvm.org/docs/CodingStandards.html#commenting

This revision is now accepted and ready to land.Jun 16 2016, 9:39 AM
JamesReynolds edited edge metadata.

Fixed nits.

JamesReynolds marked 2 inline comments as done.Jun 17 2016, 1:06 AM
JamesReynolds added inline comments.
clang-tidy/readability/IdentifierNamingCheck.h
98 ↗(On Diff #61071)

Fixed this one too.

JamesReynolds marked an inline comment as done.Jun 17 2016, 1:08 AM

Thanks! All fixed now. Could you land this for me please?

Thanks! All fixed now. Could you land this for me please?

Sure, will do. Thank you for working on this!

This revision was automatically updated to reflect the committed changes.