add isAtPosition narrowing matcher for parmVarDecl

[oontvoo]

P.S: Please ignore the child revision ... accidentally committed it to the wrong branch. I've fixed the git branches locally but can't figure out how to tell phabricator ...

[gribozavr2]

Please run clang/docs/tools/dump_ast_matchers.py to update the docs.

clang/include/clang/ASTMatchers/ASTMatchers.h

7053

Could you move it closer to other parameter-related matchers, maybe near the definition of forEachArgumentWithParam?

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

2650

Could you change some tests to have a function declaration instead of definition?

[aaron.ballman]

Does the hasParameter() matcher not already do this?

[ymandel]

clang/include/clang/ASTMatchers/ASTMatchers.h

7049

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)?

7050

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.

[oontvoo]

• Added docs
• Make it work with {block,objcMethod}Decl too. More tests

[oontvoo]

In D80603#2057014, @aaron.ballman wrote:

Does the hasParameter() matcher not already do this?

Yes, it does. But we'd like it to be able to match on the parmVarDecl().

clang/include/clang/ASTMatchers/ASTMatchers.h

7049

Why [not] hasParent ?

The direct parent of the param is a TypeLoc, so I thought hasAncestor() was more convenient. But I've changed it to two nested hasParent() now.

7053

Moved it to after hasAncestor.
(Can't move to near forEachArgumentWithParam because that's before hasAncestor and hasParent, which are needed by this)

[aaron.ballman]

In D80603#2057970, @oontvoo wrote:

In D80603#2057014, @aaron.ballman wrote:

Does the hasParameter() matcher not already do this?

Yes, it does. But we'd like it to be able to match on the parmVarDecl().

I'm sorry if I'm being dense, but hasParameter traverses to the ParmVarDecl, so I'm not certain I understand why this new matcher is needed as a public matcher. It seems like you can already accomplish this without adding a second API to do effectively the same thing: functionDecl(hasParameter(0, parmVarDecl().bind("param"))), can't you?

[oontvoo]

Remove unrelated doc generated by the script

[oontvoo]

In D80603#2058019, @aaron.ballman wrote:

In D80603#2057970, @oontvoo wrote:

In D80603#2057014, @aaron.ballman wrote:

Does the hasParameter() matcher not already do this?

Yes, it does. But we'd like it to be able to match on the parmVarDecl().

I'm sorry if I'm being dense, but hasParameter traverses to the ParmVarDecl, so I'm not certain I understand why this new matcher is needed as a public matcher. It seems like you can already accomplish this without adding a second API to do effectively the same thing: functionDecl(hasParameter(0, parmVarDecl().bind("param"))), can't you?

Ah, I didn't mean to say it wasn't *already possible* to do any of this.

It would be *very* nice to have this (at least to some of us, that I know of). (make the logic a lot simpler in a few custom matchers).

[oontvoo]

use helper for examining the node instead of nested matchers

[oontvoo]

More tests for Block

[gribozavr2]

In D80603#2058019, @aaron.ballman wrote:

I'm sorry if I'm being dense, but hasParameter traverses to the ParmVarDecl, so I'm not certain I understand why this new matcher is needed as a public matcher. It seems like you can already accomplish this without adding a second API to do effectively the same thing: functionDecl(hasParameter(0, parmVarDecl().bind("param"))), can't you?

It is syntax sugar, true. Note that the proposed matcher is a narrowing matcher for parmVarDecl(), while your suggestion is a narrowing matcher for functionDecl(), so it is not an entirely apples-to-apples comparison. Think about use cases like: declRefExpr(to(parmVarDecl(at(...)))). To rewrite that with hasParameter(), we have to use hasAncestor to find the functionDecl first, and then compare the AST node pointers. So while it is possible to express this proposed operation today, it requires a complex matcher for such a conceptually simple operation.

clang/include/clang/ASTMatchers/ASTMatchers.h

4621

Identifiers with double underscores are reserved. Also, this file typically does not define helpers.

My best suggestion is to define a local lambda in the AST_MATCHER_P, or just copy the code three times -- there is not that much duplication.

[gribozavr2]

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

2672

Please use matchesObjC / notMatchesObjC.

[oontvoo]

Change tests to us no/matchesObjC

[aaron.ballman]

In D80603#2059122, @gribozavr2 wrote:

In D80603#2058019, @aaron.ballman wrote:

I'm sorry if I'm being dense, but hasParameter traverses to the ParmVarDecl, so I'm not certain I understand why this new matcher is needed as a public matcher. It seems like you can already accomplish this without adding a second API to do effectively the same thing: functionDecl(hasParameter(0, parmVarDecl().bind("param"))), can't you?

It is syntax sugar, true. Note that the proposed matcher is a narrowing matcher for parmVarDecl(), while your suggestion is a narrowing matcher for functionDecl(), so it is not an entirely apples-to-apples comparison. Think about use cases like: declRefExpr(to(parmVarDecl(at(...)))). To rewrite that with hasParameter(), we have to use hasAncestor to find the functionDecl first, and then compare the AST node pointers. So while it is possible to express this proposed operation today, it requires a complex matcher for such a conceptually simple operation.

Thank you, that clarifies the need nicely for me!

Is there a reason we want to support blocks but not lambdas?

Also, you need to update Registry.cpp to add this to the list of dynamic matchers.

[gribozavr2]

Is there a reason we want to support blocks but not lambdas?

Lambdas are supported (see tests). Lambdas generate classes with regular function decls, so there's no surprise there.

[gribozavr2]

Also, you need to update Registry.cpp to add this to the list of dynamic matchers.

Good point. @oontvoo

[oontvoo]

Updated Registry

[oontvoo]

In D80603#2061131, @aaron.ballman wrote:

In D80603#2059122, @gribozavr2 wrote:

In D80603#2058019, @aaron.ballman wrote:

I'm sorry if I'm being dense, but hasParameter traverses to the ParmVarDecl, so I'm not certain I understand why this new matcher is needed as a public matcher. It seems like you can already accomplish this without adding a second API to do effectively the same thing: functionDecl(hasParameter(0, parmVarDecl().bind("param"))), can't you?

It is syntax sugar, true. Note that the proposed matcher is a narrowing matcher for parmVarDecl(), while your suggestion is a narrowing matcher for functionDecl(), so it is not an entirely apples-to-apples comparison. Think about use cases like: declRefExpr(to(parmVarDecl(at(...)))). To rewrite that with hasParameter(), we have to use hasAncestor to find the functionDecl first, and then compare the AST node pointers. So while it is possible to express this proposed operation today, it requires a complex matcher for such a conceptually simple operation.

Thank you, that clarifies the need nicely for me!

Is there a reason we want to support blocks but not lambdas?

Yes, the implementation does support lambdas

Also, you need to update Registry.cpp to add this to the list of dynamic matchers.

Done

[oontvoo]

Move matcher to near forEachArgumentWithParam

[oontvoo]

Thanks, all!
Committed https://github.com/llvm/llvm-project/commit/51401a676c036f2bd4e6b4b38f3538615799de40

clang/docs/LibASTMatchersReference.html

But given
   template <typename T>  class X {}; class A {};
   template <> class X<A> {}; X<A> x;
 cxxRecordDecl(hasName("::X"), isTemplateInstantiation())
   does not match, as X<A> is an explicit template specialization.
 
 Usable as: Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;, Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1VarDecl.html">VarDecl</a>&gt;, Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html">CXXRecordDecl</a>&gt;
 </pre></td></tr>
 
+
+<tr><td>Matcher&lt;clang::ParmVarDecl&gt;</td><td class="name" onclick="toggle('isAtPosition0')"><a name="isAtPosition0Anchor">isAtPosition</a></td><td>unsigned N</td></tr>
+<tr><td colspan="4" class="doc" id="isAtPosition0"><pre>Matches the ParmVarDecl nodes that are at the N'th position in the parameter
+list. The parameter list could be that of either a block, function, or
+objc-method.
+
+
+Given
+
+void f(int a, int b, int c) {
+}
+
+``parmVarDecl(isAtPosition(0))`` matches ``int a``.
+
+``parmVarDecl(isAtPosition(1))`` matches ``int b``.
+</pre></td></tr>
+
 <!--END_NARROWING_MATCHERS -->
 </table>
 
 <!-- ======================================================================= -->
 <h2 id="traversal-matchers">AST Traversal Matchers</h2>
 <!-- ======================================================================= -->
 
 <p>Traversal matchers specify the relationship to other nodes that are

clang/include/clang/ASTMatchers/ASTMatchers.h

 AST_MATCHER_P(IfStmt, hasConditionVariableStatement,
               internal::Matcher<DeclStmt>, InnerMatcher) {
   const DeclStmt* const DeclarationStatement =
     Node.getConditionVariableDeclStmt();
   return DeclarationStatement != nullptr &&
          InnerMatcher.matches(*DeclarationStatement, Finder, Builder);
 }
 
+/// Matches the ParmVarDecl nodes that are at the N'th position in the parameter
+/// list. The parameter list could be that of either a block, function, or
+/// objc-method.
+///
+///
+/// Given
+///
+/// \code
+/// void f(int a, int b, int c) {
+/// }
+/// \endcode
+///
+/// ``parmVarDecl(isAtPosition(0))`` matches ``int a``.
+///
+/// ``parmVarDecl(isAtPosition(1))`` matches ``int b``.
+AST_MATCHER_P(clang::ParmVarDecl, isAtPosition, unsigned, N) {
+  constexpr char Name[] = "##isAtPosition_parmVar_";
+
+  // The direct parent of any parmVarDecl is a TypeLoc, and not a Decl,
+  // so we have to go up one more level.
+  return parmVarDecl(
+             hasParent(typeLoc(anyOf(hasParent(blockDecl(hasParameter(
+                                         N, parmVarDecl().bind(Name)))),
+                                     hasParent(objcMethodDecl(hasParameter(
+                                         N, parmVarDecl().bind(Name)))),
+                                     hasParent(functionDecl(hasParameter(
+                                         N, parmVarDecl().bind(Name))))))),
+             equalsBoundNode(Name))
+      .matches(Node, Finder, Builder);
+}
+
 /// Matches the index expression of an array subscript expression.
 ///
 /// Given
 /// \code
 ///   int i[5];
 ///   void f() { i[1] = 42; }
 /// \endcode
 /// arraySubscriptExpression(hasIndex(integerLiteral()))

 /// Given
 ///
 /// \code
 ///   #pragma omp parallel
 ///   #pragma omp parallel default(none)
 ///   #pragma omp taskyield
 /// \endcode
 ///
 /// ``ompExecutableDirective()`` matches ``omp parallel``,
 /// ``omp parallel default(none)`` and ``omp taskyield``.
 extern const internal::VariadicDynCastAllOfMatcher<Stmt, OMPExecutableDirective>
     ompExecutableDirective;
 
 /// Matches standalone OpenMP directives,
 /// i.e., directives that can't have a structured block.
 ///
 /// Given
 ///
 /// \code
 ///   #pragma omp parallel
 ///   {}

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

 
 TEST(HasDefaultArgument, Basic) {
   EXPECT_TRUE(matches("void x(int val = 0) {}",
                       parmVarDecl(hasDefaultArgument())));
   EXPECT_TRUE(notMatches("void x(int val) {}",
                       parmVarDecl(hasDefaultArgument())));
 }
 
+TEST(IsAtPosition, Basic) {
+  EXPECT_TRUE(matches("void x(int a) {}", parmVarDecl(isAtPosition(0))));
+  EXPECT_TRUE(matches("void x(int a, int b) {}", parmVarDecl(isAtPosition(0))));
+  EXPECT_TRUE(matches("void x(int a, int b) {}", parmVarDecl(isAtPosition(1))));
+  EXPECT_TRUE(notMatches("void x(int val) {}", parmVarDecl(isAtPosition(1))));
+
+  // Tests with function-decls
+  EXPECT_TRUE(matches("void x(int a);", parmVarDecl(isAtPosition(0))));
+  EXPECT_TRUE(matches("void x(int a, int b);", parmVarDecl(isAtPosition(0))));
+  EXPECT_TRUE(matches("void x(int a, int b);", parmVarDecl(isAtPosition(1))));
+  EXPECT_TRUE(notMatches("void x(int val);", parmVarDecl(isAtPosition(1))));
+
+  // Tests with lamdas
+  EXPECT_TRUE(
+      matches("void x() { [](int a) {};  }", parmVarDecl(isAtPosition(0))));
+  EXPECT_TRUE(matches("void x() { [](int a, int b) {}; }",
+                      parmVarDecl(isAtPosition(0))));
+  EXPECT_TRUE(matches("void x() { [](int a, int b) {}; }",
+                      parmVarDecl(isAtPosition(1))));
+  EXPECT_TRUE(
+      notMatches("void x() { [](int val) {}; }", parmVarDecl(isAtPosition(1))));
+}
+
 TEST(IsArray, Basic) {
   EXPECT_TRUE(matches("struct MyClass {}; MyClass *p1 = new MyClass[10];",
                       cxxNewExpr(isArray())));
 }
 
 TEST(HasArraySize, Basic) {
   EXPECT_TRUE(matches("struct MyClass {}; MyClass *p1 = new MyClass[10];",
                       cxxNewExpr(hasArraySize(integerLiteral(equals(10))))));
 }
 
 TEST(HasDefinition, MatchesStructDefinition) {
   EXPECT_TRUE(matches("struct x {};",