Page MenuHomePhabricator

Add forEachArgumentWithParam AST matcher
ClosedPublic

Authored by flx on Oct 17 2015, 6:42 AM.

Details

Summary

The new matcher allows users to provide a matcher for both the argument of a CallExpr/CxxConstructExpr a well as the ParmVarDecl of the argument.

Diff Detail

Repository
rL LLVM

Event Timeline

flx updated this revision to Diff 37680.Oct 17 2015, 6:42 AM
flx retitled this revision from to Add forEachArgumentWithParam AST matcher.
flx updated this object.
flx added reviewers: klimek, alexfh.
klimek added inline comments.Oct 19 2015, 2:44 AM
include/clang/ASTMatchers/ASTMatchers.h
2825–2851 ↗(On Diff #37680)

I don't think the handling of BoundNodesTreeBuilder is correct (writing tests to catch that will be a nice exercise ;)

The trick is that the resulting BoundNodesTreeBuilder must contain the full results of all matches.

If you look at the other forEach matchers, you can see a pattern:
a) create a new empty BoundNodesTreeBuilder 'Result'
b) for each match you try, create a new BoundNodesTreeBuilder with a copy of the current one
c) call match with that builder
d) for the result that matches, call addMatch

If you don't pass the builder in that is initialized with the current result, any subexpressions that uses equalsBoundNode will be incorrect.

Please add tests that test equalsBoundNode behavior in all cases, especially if you have multiple ones where everything matches, but then the equalsBoundNode fails.

flx updated this revision to Diff 37872.Oct 20 2015, 6:11 AM

I think I'm not yet fully clean on the semantics of matching bound nodes. Do we want to allow that also between the argument matcher and the param matcher by copying the matches between the two matchers or do we only support matches to bound nodes that are already matched before forEachArgumetWithParam is invoked?

I'm also not quite sure how write these tests. Looking at some of the other forEach matchers (SwitchCase, ContructorInitializer) I don't see examples I could pattern mine after.

I updated the change to copy the Builder.

klimek edited edge metadata.Oct 20 2015, 6:40 AM

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

sbenza edited edge metadata.Oct 20 2015, 12:10 PM

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

include/clang/ASTMatchers/ASTMatchers.h
2837 ↗(On Diff #37872)

use isa<> instead of dyn_cast<> if you don't care about the pointer itself.

Also, you could fold that check into the matcher itself using expr(anyOf(cxxConstructExpr(...), callExpr(...)))

alexfh removed a reviewer: alexfh.Nov 5 2015, 1:38 PM
alexfh added a subscriber: alexfh.

What's the state here?

flx updated this revision to Diff 43208.Dec 17 2015, 8:34 PM

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

Thanks. I've added that test case and am counting 4 matches, two where 'a' is used as argument and two for 'b'.

Let me know if that is sufficient whether you can think of more test cases.

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

Thanks for the example. I created a test case based on it. Let me know if any other cases are still missing.

include/clang/ASTMatchers/ASTMatchers.h
2837 ↗(On Diff #37872)

Thanks, done!

flx marked 2 inline comments as done.Dec 17 2015, 8:36 PM

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

flx updated this revision to Diff 43740.Dec 29 2015, 9:38 AM

Sam, Manuel, any other concerns?

flx updated this revision to Diff 43796.Dec 30 2015, 12:12 PM

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the first 0-th argument is actually the implicit object argument and the corresponding matcher should not be applied against parameter 0 which belongs to argument 0.

To handle this I'm incrementing the argument index by one in this case.

flx updated this revision to Diff 43797.Dec 30 2015, 12:14 PM

Sorry, I uploaded the wrong patch. This is fixed now.

flx added a comment.Dec 30 2015, 12:30 PM

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

Updated patch with this.

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

In D13845#318127, @flx wrote:

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the first 0-th argument is actually the implicit object argument and the corresponding matcher should not be applied against parameter 0 which belongs to argument 0.

To handle this I'm incrementing the argument index by one in this case.

Sorry, the above comment was confusing (a lot of off-by one errors). Corrected version:

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the 0-th argument is actually the implicit object argument and Argument matcher should not be applied it. Instead argument 1 and parameter 0 should be paired.

To handle this I'm incrementing the argument index by one in this case.

flx added a comment.Jan 14 2016, 7:05 PM

Manuel, could you take another look?

klimek accepted this revision.Jan 15 2016, 1:25 AM
klimek edited edge metadata.

LG.

unittests/ASTMatchers/ASTMatchersTest.cpp
1689–1692 ↗(On Diff #43797)

Nice!

This revision is now accepted and ready to land.Jan 15 2016, 1:25 AM
flx added a comment.Jan 17 2016, 8:13 AM

Manuel, could you submit the change? I'm still working on becoming a submitter.

klimek added inline comments.Jan 18 2016, 3:22 AM
include/clang/ASTMatchers/ASTMatchers.h
2899 ↗(On Diff #43797)

Changing to unsigned due to -Wsign-compare trigger.

This revision was automatically updated to reflect the committed changes.