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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
