[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: include/clang/ASTMatchers/ASTMatchers.h:2908
@@ -2893,1 +2907,3 @@
+/// \brief Matches \c FunctionDecls and FunctionProtoTypes that have a specific
+/// parameter count.
 ///
----------------
Can you document the expected parameter count for:
```
void f(int i, ...);
```
(I would expect it to report 1.)

================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:4022
@@ +4021,3 @@
+/// functionProtoType()
+///   matches "int (*f)(int)" and the type of "g".
+AST_TYPE_MATCHER(FunctionProtoType, functionProtoType);
----------------
That's what I was expecting; can you update the comment to match reality?

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:4282
@@ +4281,3 @@
+  EXPECT_TRUE(matches("void f(int i);", functionProtoType()));
+  EXPECT_TRUE(matches("void f();", functionProtoType(parameterCountIs(0))));
+  EXPECT_TRUE(
----------------
This isn't quite what I had in mind. The test you added is a good one to add, but I wanted a test that functionProtoType() does *not* match f() (because it has no prototype), and another test for parameterCountIs() that it does't explode on a function without a prototype in C.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:4990
@@ +4989,3 @@
+  EXPECT_TRUE(matches("typedef int hasUnderlyingTypeTest;",
+                      typedefDecl(hasUnderlyingType(asString("int")))));
+  EXPECT_TRUE(
----------------
I would expect hasUnderlyingType to look through all of the type sugar, not just the top layer of it (since existing matchers can implement the current behavior by using hasType, I believe). I think the correct approach is to get the underlying type, then loop to see whether that type matches, and if not, strip off a layer of sugar and try again. Terminate the loop when there's no more sugar to strip. The following should all match:
```
EXPECT_TRUE(
      matches("typedef int foo; typedef foo bar; typedef bar baz;",
              typedefDecl(hasUnderlyingType(asString("int")), hasName("bar"))));
EXPECT_TRUE(
      matches("typedef int foo; typedef foo bar; typedef bar baz;",
              typedefDecl(hasUnderlyingType(asString("foo")), hasName("baz"))));
EXPECT_TRUE(
      matches("typedef int foo; typedef foo bar; typedef bar baz;",
              typedefDecl(hasUnderlyingType(asString("int")), hasName("baz"))));
```
The other question I have is about qualifiers. Should this match?
```
EXPECT_TRUE(
      matches("typedef const int foo;",
              typedefDecl(hasUnderlyingType(asString("int")), hasName("foo"))));
```
Or should it require `asString("const int")`?
(Regardless of the answer, we should document the behavior and add a test with qualifiers.)


http://reviews.llvm.org/D8149