Page MenuHomePhabricator

[ASTMatchers] overload ignoringParens for Expr
ClosedPublic

Authored by JonasToth on Nov 9 2018, 6:09 AM.

Diff Detail

Repository
rC Clang

Event Timeline

JonasToth created this revision.Nov 9 2018, 6:09 AM
aaron.ballman added inline comments.Nov 9 2018, 8:38 AM
include/clang/ASTMatchers/ASTMatchers.h
814–815

Can you do this via AST_POLYMORPHIC_MATCHER_P instead, given that the implementation is the same?

Also: the patch is missing unit tests.

JonasToth added inline comments.Nov 9 2018, 10:10 AM
include/clang/ASTMatchers/ASTMatchers.h
814–815

Do you want me to add more types? e.g. TypeLoc has IgnoreParens(), too.

aaron.ballman added inline comments.Nov 9 2018, 10:12 AM
include/clang/ASTMatchers/ASTMatchers.h
814–815

I'd not be opposed, given that we already expose the typeLoc() matcher. I'll leave that to your discretion.

JonasToth updated this revision to Diff 173391.Nov 9 2018, 11:16 AM
JonasToth marked 3 inline comments as done.
  • add unit test
include/clang/ASTMatchers/ASTMatchers.h
814–815

as discussed on IRC making it an AST_POLYMORPHIC_MATCHER_P does not seem to work, as the polymorphism is only in the return type. We do need the Node itself to be polymorphic (same type as returntype). The only working version I got was using the overloads.

JonasToth marked an inline comment as done.Nov 9 2018, 11:17 AM

Adding a few other reviewers in case they have ideas on how to use the polymorphic matcher rather than the overload. If they don't have a better idea, then I think the overload approach is fine.

sbenza added inline comments.Nov 9 2018, 11:47 AM
include/clang/ASTMatchers/ASTMatchers.h
814–815

You can't use AST_POLYMORPHIC_MATCHER_P to overload on input types.
You could try using templates, but that will make registering the matcher harder.
Another one that does input+output polymorphism, id, is simply not supported dynamically.

aaron.ballman accepted this revision.Nov 9 2018, 11:52 AM

LGTM!

include/clang/ASTMatchers/ASTMatchers.h
814–815

Good to know, thank you!

This revision is now accepted and ready to land.Nov 9 2018, 11:52 AM
JonasToth added inline comments.Nov 9 2018, 12:23 PM
include/clang/ASTMatchers/ASTMatchers.h
814–815

thanks for clarifying.

JonasToth retitled this revision from [clang] overload ignoringParens for Expr to [ASTMatchers] overload ignoringParens for Expr.Nov 9 2018, 12:25 PM
JonasToth added a project: Restricted Project.
JonasToth updated this revision to Diff 173422.Nov 9 2018, 12:54 PM
  • add unit test
This revision was automatically updated to reflect the committed changes.