[PATCH] D80603: add isAtPosition narrowing matcher for parmVarDecl

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7018
+AST_MATCHER_P(clang::ParmVarDecl, isAtPosition, unsigned, N) {
+  return parmVarDecl(hasAncestor(functionDecl(
+                         hasParameter(N, parmVarDecl().bind("this_decl")))),
----------------
I think that `hasAncestor` will keep going up the chain of ancestors if it fails the match on the first surrounding `functionDecl`. That could make this matcher very expensive.  Why hasParent (or, whatever combination of `hasParent` and other matchers needed to precisely describe the immediately surrounding `functionDecl`)?


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7019
+  return parmVarDecl(hasAncestor(functionDecl(
+                         hasParameter(N, parmVarDecl().bind("this_decl")))),
+                     equalsBoundNode("this_decl"))
----------------
Note that "this_decl" might be in use the by the caller (unlikely, but possible).  I'd suggest something less user friendly instead (for example, prefixing with the name of the matcher). That said, any binding inside a manager runs risk of conflict, because we don't have any scoping or fresh-name generation facilities, so any name will run some risk.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80603