[PATCH] D102213: [ASTMatchers] Add forCallable(), a generalization of forFunction().

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D102213#2755963 <https://reviews.llvm.org/D102213#2755963>, @NoQ wrote:

> I totally agree that changing forFunction() to work correctly is the right solution but backwards compatibility breakage is very real because forFunction() accepts a Matcher<FunctionDecl> whereas forCallable() accepts Matcher<Decl>

Yeah, I see now, it's not that straightforward.
Parameter type difference can be combated even keeping backward compatibility (so to speak). You can do `AST_MATCHER_P_OVERLOAD` (like it's done for `hasPrefix`, for example) to have two overloads of `forFunction`: one for `Matcher<FunctionDecl>` (aka old implementation) and one for `Matcher<Decl>` for new implementation.  I didn't check it, but it probably should work.  It is not elegant though and I'm not pushing it.

> The breakage is loud; the code will no longer compile when the intermediate decl() (or namedDecl(), or whatever) is not present. The more annoying part is that when you add namedDecl() back (or if you had it spelled out this way from the start, which doesn't make much sense but is valid and shorter than spelling out functionDecl()), your Node.get<FunctionDecl>() in the match callback will silently start returning null (on anything that isn't a functionDecl()) which may lead to unexpected crashes (previously there was no match at all, now there's a match but the node isn't of the expected type). So a relatively distant piece of code will require manual audit in order to address the potential breakage.

The breakage example has a bit weird precondition IMO.  In that scenario, the user had to use `decl` or `namedDecl` where they actually meant `functionDecl` AND they should run their matchers on code with blocks.  And even if it does happen to someone, I think it's not going to be very painful because a simple dump of the node will show what's going on.

> Additionally, forCallable(functionDecl()) is not equivalent to forFunction() (the former silently matches less stuff).

I'm not sure I understand this one.  Can you please show an example where one matches something that the other doesn't?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102213/new/

https://reviews.llvm.org/D102213