[PATCH] D8149: Add hasUnderlyingType narrowing matcher for TypedefDecls, functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:4283
@@ +4282,3 @@
+  EXPECT_TRUE(matches("void f(int i);", functionProtoType()));
+  EXPECT_TRUE(matches("void f();", functionProtoType(parameterCountIs(0))));
+  EXPECT_TRUE(notMatchesC("void f();", functionProtoType()));
----------------
aaron.ballman wrote:
> > I don't understand what would be tested by adding a test for parameterCountIs() on a function that doesn't have a prototype since it is a nested matcher passed to functionProtoType() and since the function doesn't have a prototype, that outer matcher won't match.
> 
> Not according to the AST matcher that was implemented -- it supports FunctionProtoType and FunctionDecl. I want to make sure that if you get to parameterCountIs() through functionDecl() that it doesn't fail.
Ah, OK, you were asking for a test on `functionDecl`, not `functionProtoType`.  I can do that.  I guess what threw me off is that the comment was attached to the prototype tests.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:4992
@@ +4991,3 @@
+  EXPECT_TRUE(matches("typedef int hasUnderlyingTypeTest;",
+                      typedefDecl(hasUnderlyingType(asString("int")))));
+  EXPECT_TRUE(matches("typedef const int T;",
----------------
aaron.ballman wrote:
> > I'm happy to make the improvement, but I don't know how. I simply call the Node.underlyingType(), just like hasType calls Node.getType(). I don't know why they are different.
> 
> hasType() and hasUnderlyingType() have different operational semantic implications, at least to me. hasType() asks, "ultimately, what is the type of this thing?", and Node.underlyingType() answers that. To me, hasUnderlyingType() asks, "in the chain of types that this typedef refers to, does this thing match any of them?", and Node.underlyingType() does not answer that -- it only looks at the final desugared type. The difference we want is that hasType() continues to look at the final type, but hasUnderlyingType() needs to do more work to look at intermediary types and terminate that loop when the answer is "yes, it has this type" or "no, we can't desugar any further."
> 
> If we don't care about the intermediate types for your needs, then I don't see the point to hasUnderlyingType() being an AST matcher at all. hasType() does exactly what is needed, and you can instead modify that to accept a TypedefDecl in addition to Expr and ValueDecl. However, I still see value in hasUnderlyingType() because typedef chains can be long and complex (like in the Win32 APIs), and being able to query for intermediary types would be useful. e.g., I want to know about all types that have an intermediary type of DWORD (which itself is a typedef for an unsigned integer type). hasType() will always go to the final type, making such a matcher impossible without something like hasUnderlyingType().
I just named the matcher after the [accessor](http://clang.llvm.org/doxygen/classclang_1_1TypedefNameDecl.html#a5fccedff6d3854db365a540145029158) on a typedefDecl node.  To me it's just a narrowing matcher on that node that gives you access to whatever the accessor returns.

Things may have changed in the meantime, but when I originally created this patch you couldn't do `typedefDecl(hasType(asString("int")))`.  It doesn't compile.  Since `hasType` just matched against the value of whatever `Node.getType()` returned it seemed natural to add a matcher called `hasUnderlyingType` that just matched against the value of whatever `Node.getUnderlyingType()` returned.

As I said, I'm happy to make the suggested improvement, but beyond simple immitation of existing matchers, I have no idea how to traverse the encodings of types within the guts of clang.  This is what I mean when I say "I don't know how".  I understand conceptually what you are saying but I have no idea how to express that in clang's code base.


http://reviews.llvm.org/D8149