Page MenuHomePhabricator

[ASTMatchers] Add matchers canReferToDecl() and hasUnderlyingDecl()
ClosedPublic

Authored by mboehme on Aug 1 2016, 12:57 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mboehme updated this revision to Diff 66283.Aug 1 2016, 12:57 AM
mboehme retitled this revision from to [ASTMatchers] Add matchers canReferToDecl() and hasUnderlyingDecl().
mboehme updated this object.
mboehme added reviewers: aaron.ballman, sbenza.
mboehme added a subscriber: cfe-commits.

Retracting from review for the moment. Tests fail to compile (embarrasingly enough, I ran the wrong test suite -- apologies).

mboehme updated this revision to Diff 66291.Aug 1 2016, 2:52 AM
mboehme added reviewers: sbenza, aaron.ballman.

Updated test and documentation for hasUnderlyingDecl() to use using declarations instead of typedefs. (I made the mistaken assumption that getUnderlyingDecl() would also work on a typdef and didn't catch the mistake because I ran the wrong tests.)

alexfh added a subscriber: alexfh.Aug 1 2016, 10:11 AM
alexfh added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
2440 ↗(On Diff #66291)

nit: Parentheses are superfluous here.

2807 ↗(On Diff #66291)

Please add an unresolvedLookupExpr that this matcher doesn't match (maybe change the matcher to make it easier).

aaron.ballman added inline comments.Aug 2 2016, 11:34 AM
include/clang/ASTMatchers/ASTMatchers.h
2809 ↗(On Diff #66291)

I find the name of this matcher a little bit confusing. The documentation doesn't describe what the matcher does (can you please clarify the docs?). The implementation suggests that this is looking to see if the given decl exists in the overload expression set, which makes me wonder why this isn't implemented on the hasDeclaration() traversal matcher rather than adding a new matcher name?

klimek added inline comments.Aug 4 2016, 7:52 AM
include/clang/ASTMatchers/ASTMatchers.h
2809 ↗(On Diff #66291)

I don't like hasDeclaration, as that's so far used when there is exactly one. Something like hasAnyDeclaration might also fit, but I think canReferToDecl makes it clear that there is not yet a specific decl, but a couple of candidates we'll want to select from.

mboehme updated this revision to Diff 66808.Aug 4 2016, 8:20 AM

I've rebased the patch to a newer head revision, which seems to confuse Phabricator when diffing against earlier versions of the patch. Apologies. I assume this means I should avoid doing this (rebasing to a newer revision)?

mboehme marked an inline comment as done.Aug 4 2016, 8:33 AM
mboehme added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
2486 ↗(On Diff #66808)

I was trying to match the style of the next matcher below -- or do you think I should establish a precedent / preference for emitting the superfluous parentheses?

2855 ↗(On Diff #66808)

The documentation doesn't describe what the matcher does (can you please clarify the docs?).

I've rephrased the description of the matcher -- is it clearer now?

The implementation suggests that this is looking to see if the given decl exists in the overload expression set, which makes me wonder why this isn't implemented on the hasDeclaration() traversal matcher rather than adding a new matcher name?

As klimek notes, the difference is that hasDeclaration() is currently used only for nodes that have exactly one associated declaration.

My proposal would be to stay with canReferToDecl -- thoughts?

alexfh added a comment.Aug 4 2016, 8:34 AM

I assume this means I should avoid doing this (rebasing to a newer revision)?

No, rebasing is totally fine. But if some files changed in the meantime, comparing with older revisions will contain unrelated changes, that's expected, I think.

alexfh added inline comments.Aug 4 2016, 8:37 AM
include/clang/ASTMatchers/ASTMatchers.h
2486 ↗(On Diff #66808)

Yes, I consider parentheses with return are confusing - I'm immediately trying to find a reason, why they are needed.

2855 ↗(On Diff #66808)

+1 to canReferToDecl

mboehme updated this revision to Diff 66815.Aug 4 2016, 9:09 AM

Removed superfluous parentheses

mboehme marked 3 inline comments as done.Aug 4 2016, 9:10 AM
aaron.ballman added inline comments.Aug 4 2016, 9:11 AM
include/clang/ASTMatchers/ASTMatchers.h
2855 ↗(On Diff #66808)

canReferToDecl doesn't make it any more clear that there could be multiple candidates, and I find its usage to read really strangely. However, it is a good point about hasDeclaration usage being singular currently. My preference is for hasAnyDeclaration, and the precedence is hasAnyName, hasAnyConstructorInitializer, hasAnyArgument, etc.

The documentation does read better now, thank you!

mboehme updated this revision to Diff 66912.Aug 5 2016, 12:41 AM
  • Rename canReferToDecl to hasAnyDeclaration
mboehme marked 5 inline comments as done.Aug 5 2016, 12:43 AM
aaron.ballman accepted this revision.Aug 8 2016, 5:32 AM
aaron.ballman edited edge metadata.

Assuming others are fine with hasAnyDeclaration() vs canReferToDecl(), LGTM! If others feel strongly about canReferToDecl() instead, it still LGTM.

This revision is now accepted and ready to land.Aug 8 2016, 5:32 AM
alexfh accepted this revision.Aug 8 2016, 5:02 PM
alexfh added a reviewer: alexfh.

LG

This revision was automatically updated to reflect the committed changes.