[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D116328#3223299 <https://reviews.llvm.org/D116328#3223299>, @aaron.ballman wrote:

> In D116328#3223268 <https://reviews.llvm.org/D116328#3223268>, @LegalizeAdulthood wrote:
>
>> In D116328#3221995 <https://reviews.llvm.org/D116328#3221995>, @aaron.ballman wrote:
>>
>>> 
>
> `has()` matches the direct descendant AST node, whereas `hasDescendant()` matches *any* descendant AST node.
> [...] I don't know of any performance concerns with `has()` though.

Thanks for the explanation!

>> Honestly, at this point, I become lost in manually tracing the code through "go to definition" in my IDE.
>
> I'm impressed you got this far -- the AST matching machinery under the hood is really quite complex! :-)

Having CMake generate a VS project and drilling in with ReSharper for C++ is
pretty powerful :), but you have to manually keep track of how the template
and function arguments are being resolved in the dynamic execution.  So it's
pretty easy to get lost in template oriented code, which is understandably
pretty common in AST land.

> My understanding (and my recollection here may be wrong) is that `has()` can memoize the results
> [...] but you can't get the same behavior from `hasDescendant()` or `hasAncestor()`

OK, that makes sense.

> I'm adding @sammccall in case he has information about the performance characteristics or thoughts about
> the proposed matcher in general.

So let me summarize what we've learned so far:

- `has` only descends one level, is memoizable, and is less expensive than `hasDescendant`
- `has` uses the Visitor pattern to compute the result that is memoized
- @sammccall  might be aware of any performance related issues with `has`

My takeaway:

- if `has` isn't expensive, I can either ditch this public matcher or move it to be a private matcher used in my check


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

https://reviews.llvm.org/D116328