Page MenuHomePhabricator

[clang-tidy] Implement FixitHints for identifier references in IdentifierNamingCheck
ClosedPublic

Authored by berenm on Sep 22 2015, 5:24 PM.

Details

Summary

This diff requires http://reviews.llvm.org/D13079 to be applied first. I wasn't sure about how to make patch series in Phabricator, and I wanted to keep the two separate for clarity.

It looks like that most cases can be supported with this patch. I'm not totally sure about the actual coverage though. I think that the matchers are very generic, but I'm still not totally fluent with the AST.

Diff Detail

Repository
rL LLVM

Event Timeline

berenm updated this revision to Diff 35448.Sep 22 2015, 5:24 PM
berenm retitled this revision from to [clang-tidy] Implement FixitHints for identifier references in IdentifierNamingCheck.
berenm updated this object.
berenm changed the visibility from "Public (No Login Required)" to "berenm (Beren Minor)".
berenm updated this revision to Diff 35464.Sep 22 2015, 11:29 PM
berenm changed the visibility from "berenm (Beren Minor)" to "Public (No Login Required)".
berenm changed the visibility from "Public (No Login Required)" to "berenm (Beren Minor)".
berenm updated this object.Sep 22 2015, 11:31 PM
berenm updated this revision to Diff 35465.Sep 22 2015, 11:34 PM
berenm added a reviewer: alexfh.
berenm changed the visibility from "berenm (Beren Minor)" to "Public (No Login Required)".
berenm added a subscriber: cfe-commits.

Remove remaining commented code.

berenm updated this revision to Diff 35466.Sep 22 2015, 11:45 PM

Fix incorrect line removal in unit test.

berenm updated this revision to Diff 35655.Sep 24 2015, 10:42 AM

Update the diff with more context.

berenm updated this revision to Diff 35672.Sep 24 2015, 1:50 PM
  • Do not check for identifier names from system headers
  • Check for SourceLocation validity before modifying them
alexfh edited edge metadata.Sep 25 2015, 11:13 AM

Awesome! This makes the check far more usable.

See a few minor comments in-line.

clang-tidy/readability/IdentifierNamingCheck.cpp
140 ↗(On Diff #35672)

ClangTidy takes care of filtering errors. It doesn't show errors in system headers by default, but there's an option to show them, if needed (e.g. for library developers). Is there any reason to add filtering here as well?

537 ↗(On Diff #35672)

Please split return and addUsage here and in other places. It looks rather confusing to me and adds no benefits.

test/clang-tidy/readability-identifier-naming.cpp
69 ↗(On Diff #35672)

Please don't #include system headers to tests. There are multiple reasons to avoid this:

  • that won't work in some test configurations;
  • we don't control the contents of system headers, so we can't make tests reliable;
  • testing time can increase significantly due to the possibly large size of system headers.

If you need to test includes, put mock headers to test/clang-tidy/Inputs/<check-name>/ and specify this directory in the clang-tidy invocation:

// RUN: %python %S/check_clang_tidy.py %s readability-identifier-naming %t \
// RUN:   -config=... \
// RUN:   -- -std=...  -isystem=%S/Inputs/readability-identifier-naming
berenm updated this revision to Diff 35819.Sep 27 2015, 2:19 AM
berenm edited edge metadata.

Reworked the patches a little bit, addressing the comments and adding
better handling of checks in header files with corresponding unit test cases.

berenm marked 3 inline comments as done.Sep 27 2015, 2:25 AM
berenm added inline comments.
clang-tidy/readability/IdentifierNamingCheck.cpp
141 ↗(On Diff #35819)

I wasn't sure how system headers were handled, and I thought it was a waste of time to check identifiers in them. But as you say it could be useful, I rolled back the change.

From what I've seen, clang-tidy will display the warnings from incorrect declaration names in system header files when using the -system-headers and -header-filter=.* flags, but will not offer fixes for the declaration or usages in user's code. I guess editing system headers isn't going to work fine?

For user headers, warnings and fixes are emitted when -header-filter=.* is used.

Without the flag, no warnings or fixes are emitted, which is expected.

berenm marked an inline comment as done.EditedSep 27 2015, 2:27 AM

I think the latest version also addresses D13090 in a better way. The check will now let clang-tidy decide whether warnings and fixes are displayed, whether it is in or from system/user header files or in main file, and will only worry about not emitting warnings and fixes if a macro is involved in the declaration or any usage.

mgehre added a subscriber: mgehre.Sep 28 2015, 1:48 PM
alexfh added inline comments.Sep 29 2015, 5:38 AM
clang-tidy/readability/IdentifierNamingCheck.cpp
537 ↗(On Diff #35819)

There are cases where this will fail (~ ClassName() or ??-ClassName or ~\<EOL>ClassName), but I saw these only a couple of times in real code. In other similar cases I'd recommend using the lexer and skip to the next token, but here it seems to be an overkill.

545 ↗(On Diff #35819)

The four cases are too similar. It should be possible to write the code much shorter. This might work:

if (isa<TagTypeLoc>(Loc) || isa<InjectedClassNameTypeLoc>(Loc) || ...)
  addUsage(NamingCheckFailures, Loc->getType()->getDecl(),
      Loc->getSourceRange(), Result.SourceManager);
575 ↗(On Diff #35819)

Please remove = SourceRange.

578 ↗(On Diff #35819)

Can you just cast to TemplateDecl or RedeclarableTemplateDecl, whichever suits better?

berenm added inline comments.Sep 29 2015, 7:00 AM
clang-tidy/readability/IdentifierNamingCheck.cpp
545 ↗(On Diff #35819)

It doesn't look to work well with TypeLoc. This looks to be a convoluted class hierarchy (http://clang.llvm.org/doxygen/classclang_1_1TypeLoc.html) and getDecl() is only available on some derived TypeLoc classes.

I can change to something smaller, is that any better?

NamedDecl *Decl = nullptr;
if (const auto &Ref = Loc->getAs<TagTypeLoc>()) {
  Decl = Ref.getDecl();
} else if (const auto &Ref = Loc->getAs<InjectedClassNameTypeLoc>()) {
  Decl = Ref.getDecl();
} else if (const auto &Ref = Loc->getAs<UnresolvedUsingTypeLoc>()) {
  Decl = Ref.getDecl();
} else if (const auto &Ref = Loc->getAs<TemplateTypeParmTypeLoc>()) {
  Decl = Ref.getDecl();
}

if (Decl) {
  addUsage(NamingCheckFailures, Decl, Loc->getSourceRange(),
           Result.SourceManager);
  return;
}
berenm updated this revision to Diff 35972.Sep 29 2015, 7:29 AM

Address the latest comments, as well as useless code removal.

  • Only the start of the token range is stored, so there is no need to care about the end of the range. Remove the code that was trying to match the end of the range (template, nested name).
  • Add some spaces in test cases (destructors, templates) just to be sure that everything works as expected.
  • Add a missing CHECK-FIXES in the test case on the abstract_class destructor.
berenm updated this revision to Diff 35974.Sep 29 2015, 7:31 AM

Fix arcanist messup...

berenm marked 3 inline comments as done.Sep 29 2015, 7:33 AM
jbcoe added a subscriber: jbcoe.Sep 30 2015, 3:15 PM
alexfh accepted this revision.Oct 1 2015, 2:08 AM
alexfh edited edge metadata.

The patch looks good with one comment. I'll fix it and submit the patch for you.

Thank you for working on this!

clang-tidy/readability/IdentifierNamingCheck.cpp
548 ↗(On Diff #35974)

Note, that I suggested to use Loc->getType()->getDecl(), which uses TypeLoc::getType() and Type::getDecl().

This revision is now accepted and ready to land.Oct 1 2015, 2:08 AM
alexfh added inline comments.Oct 1 2015, 2:20 AM
clang-tidy/readability/IdentifierNamingCheck.cpp
548 ↗(On Diff #35974)

Ah, I see now: the problem is not TypeLoc here, it's that there's no Type::getDecl. So your version is probably the best we can do here.

This revision was automatically updated to reflect the committed changes.
alexfh added a comment.Oct 1 2015, 2:35 AM

Also note, that in October I'll be mostly unavailable, but other clang-tidy contributors can do the review and help getting patches in: sbenza, bkramer, aaron.ballman.

berenm added a comment.Oct 1 2015, 2:36 AM

Alright, thank you for your time!