Page MenuHomePhabricator

[clang-tidy] Fix a template function false positive in misc-unused-using-decls check.
ClosedPublic

Authored by hokein on May 17 2016, 6:15 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 57470.May 17 2016, 6:15 AM
hokein retitled this revision from to [clang-tidy] Fix a template function false positive in misc-unused-using-decls check..
hokein updated this object.
hokein set the repository for this revision to rL LLVM.
hokein added a subscriber: cfe-commits.
alexfh added inline comments.May 17 2016, 8:00 AM
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
24 ↗(On Diff #57470)

I think, we should use a node matcher for UnresolvedLookupExpr and instead use callExpr(callee(unresolvedLookupExpr().bind("something"))).

Also, I would create the new matcher in the ASTMatchers.h right away, together with a test and docs. But if you want this patch to be in soon, feel free to move the matcher to the matchers library in a follow up.

alexfh requested changes to this revision.May 17 2016, 8:00 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.May 17 2016, 8:00 AM
hokein updated this revision to Diff 57496.May 17 2016, 9:58 AM
hokein edited edge metadata.

Address review comments.

hokein marked an inline comment as done.May 17 2016, 9:59 AM
hokein added inline comments.
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
24 ↗(On Diff #57470)

Agree. I will add a node matcher for UnresolvedLookupExpr in a follow-up patch.

alexfh requested changes to this revision.May 17 2016, 11:52 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
24 ↗(On Diff #57496)

Sorry for being unclear. Please change this to the node matcher in this patch. You can move the matcher to the ASTMatchers.h as a follow-up. hasUnresolvedLookupExpr is not in line with the matchers design: the CallExpr node has no property UnresolvedLookupExpr. You'll also be able to bind the UnresolvedLookupExpr to an identifier and make the code in check() slightly shorter.

This revision now requires changes to proceed.May 17 2016, 11:52 AM
hokein updated this revision to Diff 57569.May 18 2016, 1:04 AM
hokein edited edge metadata.
hokein marked an inline comment as done.

Make a node matcher for unresolvedLookupExpr type.

hokein marked an inline comment as done.May 18 2016, 1:06 AM
hokein added inline comments.
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
24 ↗(On Diff #57496)

Done. But currently we can't bind the UnresolvedLookupExpr directly since the AST_MATCHER doesn't provide a bind function here.

alexfh added inline comments.May 18 2016, 1:30 AM
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
24 ↗(On Diff #57569)

That's because we need a node matcher, not narrowing matcher. I guess, this should work:

const internal::VariadicAllOfMatcher<UnresolvedLookupExpr> unresolvedLookupExpr;
hokein marked an inline comment as done.May 18 2016, 1:35 AM
hokein added inline comments.
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
24 ↗(On Diff #57569)

Yeah. But as the name indicates, VariadicAllOfMatcher is used internally in ASTMatcher, is it reasonable to use it here? I can't find such usage in clang-tidy side.

alexfh added inline comments.May 18 2016, 1:45 AM
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
24 ↗(On Diff #57569)

There are far fewer nodes in the AST than their properties, so the core matchers library should contain all node matchers. Thus, the user code is not supposed to create any node matchers and the corresponding API is considered an implementation detail of the matchers library. It's fine to use internal API here for a short period until we move the matcher to the core matchers library.

hokein updated this revision to Diff 57576.May 18 2016, 2:06 AM
hokein edited edge metadata.

Create a node matcher for UnresolvedLookupExpr.

hokein marked an inline comment as done.May 18 2016, 2:07 AM
hokein added inline comments.
clang-tidy/misc/UnusedUsingDeclsCheck.cpp
24 ↗(On Diff #57576)

Sounds good. Done.

alexfh accepted this revision.May 18 2016, 2:39 AM
alexfh edited edge metadata.

LG

clang-tidy/misc/UnusedUsingDeclsCheck.cpp
95 ↗(On Diff #57576)

Range-based for loop, maybe?

This revision is now accepted and ready to land.May 18 2016, 2:39 AM
hokein updated this revision to Diff 57586.May 18 2016, 4:20 AM
hokein marked an inline comment as done.
hokein edited edge metadata.

Use for-range loop.

hokein marked an inline comment as done.May 18 2016, 4:20 AM
This revision was automatically updated to reflect the committed changes.