Page MenuHomePhabricator

Support variables and functions types in misc-unused-using-decls.
ClosedPublic

Authored by hokein on May 6 2016, 6:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 56411.May 6 2016, 6:28 AM
hokein retitled this revision from to Support variables and functions types in misc-unused-using-decls..
hokein updated this object.
hokein updated this revision to Diff 56412.May 6 2016, 6:31 AM

Fix code style.

hokein updated this object.May 6 2016, 6:32 AM
hokein added reviewers: alex, djasper.
hokein set the repository for this revision to rL LLVM.
hokein added a subscriber: cfe-commits.
djasper added inline comments.May 8 2016, 11:57 AM
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
63 ↗(On Diff #56412)

Use early return here instead of "else".

65 ↗(On Diff #56412)

No braces for single statement ifs.

68 ↗(On Diff #56412)

Wouldn't you also want to do this via the canonical decl? Add a test.

71 ↗(On Diff #56412)

Would it be important to look at something like the canonical decl here? Can you add a test with a (static) class variable that is initialized out of line?

test/clang-tidy/misc-unused-using-decls.cpp
60 ↗(On Diff #56412)

Can you add tests where variables and functions are solely referenced by pointers to members? Compare:
http://reviews.llvm.org/D20054

hokein updated this revision to Diff 56540.May 9 2016, 2:30 AM
hokein marked 5 inline comments as done.
  • Address review comments.
  • Ignore using-declarations defined in class definition.
hokein added a comment.May 9 2016, 2:46 AM

I ran the check on LLVM project. The check found 8 hits: http://reviews.llvm.org/D20060 (Check all of them, no false positives).

djasper added inline comments.May 9 2016, 3:03 AM
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
39 ↗(On Diff #56540)

We should also ignore using declarations in function context (Koenig lookup..).

clang-tidy/misc/UnusedUsingDeclsCheck.h
33 ↗(On Diff #56540)

nit: Add an empty line after this.

test/clang-tidy/misc-unused-using-decls.cpp
27 ↗(On Diff #56540)

Move this into the "Using declarations" section.

hokein updated this revision to Diff 56555.May 9 2016, 5:12 AM
hokein marked 2 inline comments as done.

Update.

clang-tidy/misc/UnusedUsingDeclsCheck.cpp
39 ↗(On Diff #56540)

If we ignore them in function context, we will also skip the following case:

namespace a {
class A {};
};

void func() {
  using a::A; // unused-using decls.
}
alex removed a reviewer: alex.May 9 2016, 5:15 AM
djasper accepted this revision.May 9 2016, 5:26 AM
djasper edited edge metadata.

Looks good.

clang-tidy/misc/UnusedUsingDeclsCheck.cpp
39 ↗(On Diff #56555)

Ack. I think this is actually ok for now.

66 ↗(On Diff #56555)

Maybe the ->getCanonicalDecl() should be moved into removeFromFoundDecls?

This revision is now accepted and ready to land.May 9 2016, 5:26 AM
hokein updated this revision to Diff 56566.May 9 2016, 6:22 AM
hokein edited edge metadata.

Address remaining comments.

hokein marked an inline comment as done.May 9 2016, 6:23 AM
This revision was automatically updated to reflect the committed changes.