Page MenuHomePhabricator

Add new ASTMatcher that matches dynamic exception specifications.
ClosedPublic

Authored by hintonda on May 7 2016, 7:43 PM.

Details

Summary

Add new ASTMatcher that matches dynamic exception specifications.

Diff Detail

Event Timeline

hintonda updated this revision to Diff 56510.May 7 2016, 7:43 PM
hintonda retitled this revision from to Add new ASTMatcher that matches dynamic exception specifications..
hintonda updated this object.
hintonda added a reviewer: alexfh.
hintonda added a subscriber: cfe-commits.
alexfh edited edge metadata.May 8 2016, 12:06 AM

Please add tests, re-generate documentation and add the matcher to the dynamic matchers registry. See http://reviews.llvm.org/D19871 for an example.

alexfh edited reviewers, added: klimek, sbenza; removed: alexfh.May 8 2016, 12:07 AM
alexfh removed a subscriber: klimek.
hintonda updated this revision to Diff 56522.May 8 2016, 1:09 PM
  • Added test, re-generated documentation, and add new matcher to the dynamic matchers registry.
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
3229

It's a bit odd to expose this on the declaration instead of the type since the AST carries this information on the type, not the declaration. I definitely see the utility in not having to go from the decl to the type in an AST matcher, though. Can you expose it on both FunctionDecl and FunctionProtoType instead?

hintonda added inline comments.May 11 2016, 11:36 AM
include/clang/ASTMatchers/ASTMatchers.h
3229

It's modeled on the isNoThrow matcher directly below which used FunctionDecl, so I used it for this one too.

Did you want me change this one to use FunctionProtoType or add 2 matchers, one using FunctionDecl and another one using FunctionProtoType?

aaron.ballman accepted this revision.May 11 2016, 12:51 PM
aaron.ballman edited edge metadata.

LGTM!

include/clang/ASTMatchers/ASTMatchers.h
3229

Ah, good point about isNoThrow. I would say leave it as FunctionDecl for now and maybe a follow-on patch (not requesting you do this work, btw) can convert it to be a polymorphic matcher that accepts FunctionDecl and FunctionProtoType.

This revision is now accepted and ready to land.May 11 2016, 12:51 PM

Great, thanks. Btw, I don't have commit access, so could you land it for me?

hintonda updated this revision to Diff 57290.May 14 2016, 6:29 PM
hintonda edited edge metadata.
  • Added new isThrow matcher to complement isNoThrow, and changed hadDynamicExceptionSpec to take a parameter, e.g., isThrow() or isNoThrow().
aaron.ballman requested changes to this revision.May 16 2016, 8:15 AM
aaron.ballman edited edge metadata.

I'm not keen on the new direction this patch has taken. hasDynamicExceptionSpec(isThrow()) is quite novel. What is the problem you are trying to solve with this?

include/clang/ASTMatchers/ASTMatchers.h
3251

I'm not comfortable with this matcher; users can use unless(isNoThrow()) to get this behavior already.

This revision now requires changes to proceed.May 16 2016, 8:15 AM

Great, unless() was what I was missing. I'll refactor along those lines.

As for what I'm trying to achieve, I want a set containing "throw()" and another set containing "throw(something)". If there's a cleaner way to achieve that with an existing matcher, please let me know.

Great, unless() was what I was missing. I'll refactor along those lines.

As for what I'm trying to achieve, I want a set containing "throw()" and another set containing "throw(something)". If there's a cleaner way to achieve that with an existing matcher, please let me know.

allOf(hasDynamicExceptionSpecification(), isNoThrow()) gets you "throw()" and allOf(hasDynamicExceptionSpecification(), unless(isNoThrow()) gets you "throw(something)", doesn't it?

Great, that's exactly what I needed. I'll revert back to the previous version.

hintonda updated this revision to Diff 57356.May 16 2016, 9:08 AM
hintonda edited edge metadata.

Revert back to previous version.

aaron.ballman accepted this revision.May 16 2016, 9:34 AM
aaron.ballman edited edge metadata.

Okay, back to looking good. :-) I will commit on your behalf, since I'm back to somewhere with source access again.

This revision is now accepted and ready to land.May 16 2016, 9:34 AM

Great, thanks again. This has been a great learning experience.

aaron.ballman closed this revision.May 16 2016, 9:55 AM

Thank you for the work on this! I've commit in r269662