Page MenuHomePhabricator

[ASTMatchers] Make isNoThrow and hasDynamicExceptionSpec polymorphic for use with both functionDecl and functionProtoType
ClosedPublic

Authored by hintonda on May 26 2016, 9:23 AM.

Details

Summary

Make isNoThrow and hasDynamicExceptionSpec polymorphic so they can be used with both functionDecl and functionPrototype matchers.

Diff Detail

Event Timeline

hintonda updated this revision to Diff 58633.May 26 2016, 9:23 AM
hintonda retitled this revision from to Update hasDynamicExceptionSpec to use functionType instead of functionDecl..
hintonda updated this object.
hintonda added a reviewer: aaron.ballman.
hintonda added a subscriber: cfe-commits.
aaron.ballman requested changes to this revision.Jun 5 2016, 10:05 AM
aaron.ballman edited edge metadata.

Sorry for the delayed response, this review got lost in my inbox!

This change will break existing out-of-tree users and would be a regression. Instead, the matcher should become a polymorphic matcher that accepts a FunctionDecl as well as a FunctionType. Check out the matcher for isConstexpr() to see how to create a polymorphic matcher. Note that you may need to use a trick like the one used in hasAnySubstatement() to handle the differences between getting the exception spec from a FunctionDecl and a FunctionType from the same matcher.

Also, can you update isNoThrow() at the same time? It has the similar behavior of currently working only on a FunctionDecl and not a FunctionType.

Finally, I'm wondering whether we want this to work on a FunctionType or a FunctionProtoType. Only functions with prototypes can have an exception specification, so putting the functionality on FunctionProtoType keeps it closer to the AST representation. However, this also may make it slightly more awkward to use in practice. Thoughts?

This revision now requires changes to proceed.Jun 5 2016, 10:05 AM

This marcher was recently added, and has never been in a release. Specifically, it was added by me in support of a checker that has now been abandoned in lieu of a better approach -- the new approach requires this change -- so I doubt it would break anything.

That said, I will look into polymorphic matchers if that is the preferred solution.

This marcher was recently added, and has never been in a release. Specifically, it was added by me in support of a checker that has now been abandoned in lieu of a better approach -- the new approach requires this change -- so I doubt it would break anything.

That's good to know, thanks!

That said, I will look into polymorphic matchers if that is the preferred solution.

Thank you -- I think that is still the right approach to take. We try to keep the matchers as close to the AST representation as possible, and this one (plus isNoThrow()) slipped through the cracks (in a useful way).

hintonda retitled this revision from Update hasDynamicExceptionSpec to use functionType instead of functionDecl. to [ASTMatchers] Make isNoThrow and hasDynamicExceptionSpec polymorphic for use with both functionDecl and functionProtoType.Jun 6 2016, 6:33 AM
hintonda updated this object.
hintonda edited edge metadata.
hintonda updated this revision to Diff 59726.Jun 6 2016, 8:26 AM
hintonda edited edge metadata.

Update narrowing matchers, isNoThrow and hasDynamicExceptionSpec, to
work with both functionDecl and functionProtoType matchers. Update
tests and regenerate documentation.

Since FunctionDecl and FunctionProtoType are not polymorphically
related, an overloaded helper function, getFunctionProtoType(), was
added that takes either a FunctionDecl node or FunctionProtoType node
and returns a FunctionProtoType pointer. This is based on the
getUnderlyingType() implementation.

This is looking good, aside from a few small nits. The only question I have remaining is whether this should be on FunctionProtoType or FunctionType? I think being on FunctionProtoType (how you have it) is preferable. @sbenza or @klimek, do you agree?

include/clang/ASTMatchers/ASTMatchers.h
3254

Since the type isn't explicitly spelled out in the initialization, probably should use const FunctionProtoType * instead of const auto *.

3274

Same here.

hintonda updated this revision to Diff 59747.Jun 6 2016, 10:21 AM
hintonda edited edge metadata.

Use FunctionProtoType instead of auto for clarity.

aaron.ballman accepted this revision.Jun 7 2016, 7:20 AM
aaron.ballman edited edge metadata.

LGTM, thank you for working on this!

This revision is now accepted and ready to land.Jun 7 2016, 7:20 AM

Thanks Aaron. If you could commit for me, I'd appreciate it. Thanks again...

aaron.ballman closed this revision.Jun 7 2016, 10:41 AM

Committed in r272028.