[PATCH] D19357: [ASTMatchers] New matcher forFunction

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Thanks for doing this!

I think we want the version that iterates all parents.
Otherwise it will have problems with templates.
That is, you won't know which `FunctionDecl` you will get: the template or the instantiation. And you might need one or the other specifically.


================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:5124
@@ +5123,3 @@
+///   but does match 'return > 0'
+AST_MATCHER_P(Stmt, forFunction, internal::Matcher<FunctionDecl>,
+              InnerMatcher) {
----------------
I think this matcher works for Decls too, but we can leave it like this until we need the polymorphism.

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:5127
@@ +5126,3 @@
+  const auto &Parents = Finder->getASTContext().getParents(Node);
+  assert(!Parents.empty() && "Found node that is not in the parent map.");
+
----------------
You should not assert() this.
Parents can be empty if the statement is not inside a function. eg a global variable initializer.

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:5132
@@ +5131,3 @@
+  while(!Stack.empty()) {
+    const auto &CurNode = Stack.back();
+    Stack.pop_back();
----------------
If you are taking from the back, use a vector<> instead.
Or SmallVector<> to avoid allocation in most cases.

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:5134
@@ +5133,3 @@
+    Stack.pop_back();
+    if(const auto *DeclNode = CurNode.get<Decl>()) {
+      if(const auto *FuncDeclNode = dyn_cast<FunctionDecl>(DeclNode)) {
----------------
Just call `CurNode.get<FunctionDecl>`. It'll do the dyn_cast for you.

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:5139
@@ +5138,3 @@
+        }
+      }
+    }
----------------
We should check for `LambdaExpr` here too, and try to match against `LambdaExpr::getCallOperator()`

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:5141
@@ +5140,3 @@
+    }
+    const auto *StmtNode = CurNode.get<Stmt>();
+    if(StmtNode&&!isa<LambdaExpr>(StmtNode)) {
----------------
You don't need to get a Stmt. getParents() works with DynTypedNodes.
There might be some non-stmt nodes in the middle, like variable declarations.

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:5142
@@ +5141,3 @@
+    const auto *StmtNode = CurNode.get<Stmt>();
+    if(StmtNode&&!isa<LambdaExpr>(StmtNode)) {
+      for(const auto &Parent: Finder->getASTContext().getParents(*StmtNode))
----------------
We should also not traverse FunctionDecls.
Eg:

```
void F() {
  struct S {
    void F2() {
    }
  };
}
```

So, on each node: if is a FunctionDecl or a LambdaExpr run the matcher, otherwise traverse.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:5581
@@ +5580,3 @@
+    "  typedef PosVec *iterator;"
+    "  typedef const PosVec *const_iterator;"
+    "  iterator begin();"
----------------
All of this is kind of unnecessary for the test.
The function could just be:

```
PosVec& operator=(const PosVec&) {
  auto x = [] { return 1; };
  return *this;
}
```

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:5596
@@ +5595,3 @@
+                 has(unaryOperator(hasOperatorName("*"))))));
+  EXPECT_FALSE(
+    matches(
----------------
`EXPECT_TRUE(notMatches(...`

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:5601
@@ +5600,3 @@
+                 has(binaryOperator(hasOperatorName(">"))))));
+                 llvm::errs()<<"d";
+}
----------------
Please add test cases for the changes suggested to the matcher.


http://reviews.llvm.org/D19357