[PATCH] D16526: Add hasRetValue narrowing matcher for returnStmt

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

In http://reviews.llvm.org/D16526#334795, @LegalizeAdulthood wrote:

> In http://reviews.llvm.org/D16526#334742, @aaron.ballman wrote:
>
> > I get the same behavior from the existing has() matcher -- how is this meant to differ?
>
>
> Hmm!  Good question.  I suppose it doesn't do anything different.
>
> I was thinking how to write such matching expressions and I looked through the matcher reference and the header file for things related to ReturnStmt and found only `returns`.  It never occurred to me to use `has`.  Not seeing returnStmt mentioned anywhere, it seemed that I couldn't write such matching expressions and needed a new matcher.


Yes, I have definitely run into this line of thinking in the past as well. Specifically, I ran into it with cxxCatchStmt (to determine what kind of type the catch statement will handle).

> So the question remains: is it bad to have a matcher that explicitly models the narrowing for a return statement's return expression when you can achieve the same thing with `has`?


I'm of two minds on this: (1) the status quo is a bit obtuse and so a dedicated matcher might not be a bad thing, (2) AST matchers are a bit expensive due to the template metaprogramming and duplicate functionality is easy to get out of sync. I am leaning towards not duplicating AST matcher functionality because of (2) and think (1) can be corrected with some better documentation for has(), returnStmt(), and perhaps cxxCatchHandler(). The fact that there are > 1 instances where has() works but we might want a dedicated matcher also suggests that we don't want to duplicate. @klimek, what is your stance on the idea?


http://reviews.llvm.org/D16526