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

[Aaron Ballman via cfe-commits]

aaron.ballman 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()));
----------------
LegalizeAdulthood wrote:
> 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.
Ah, yeah, one of the downsides to Phab sometimes is finding a good place to put comments for things that are missing. :-P Sorry for the confusion!

================
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;",
----------------
LegalizeAdulthood wrote:
> 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.
That's why I am trying to understand what your goal is. As best I can tell, there are two distinct things someone may want to do with a typedef's type: figure out what it ultimately boils down to, or traverse the typedef chain. I was uncertain which one fit your needs better, but it sounds like "get the ultimate type" is what you want. From there we can discuss what the proper design of the matcher is. I think I was thrown off by you naming it *has*UnderlyingType instead of *get*UnderlyingType because *has* implies a different relationship than *get* and I couldn't see why you wouldn't just update hasType() instead.

Another thing to think about: the typedef matcher is a bit controversial in terms of the semantics, but the other matchers are not. Perhaps it makes sense to split the patch into two parts?

As for how to actually implement the traversal, I think it looks something like (totally untested):
```
QualType QT = Context.getTypedefType(&Node);
for (QualType NextQT; QT != NextQT; NextQT = QT.getSingleStepDesugaredType(Context)) {
  // If matches, return Type
}
return QT;
```


http://reviews.llvm.org/D8149