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

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:4994
@@ +4993,3 @@
+  EXPECT_TRUE(matches("typedef int hasUnderlyingTypeTest;",
+                      typedefDecl(hasUnderlyingType(asString("int")))));
+  EXPECT_TRUE(matches("typedef const int T;",
----------------
aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > Thank you for those examples! Given the following code:
> > > > > ```
> > > > > typedef int foo;
> > > > > typedef foo bar;
> > > > > 
> > > > > bar i;
> > > > > ```
> > > > > clang-query> match varDecl(hasType(asString("int")))
> > > > > 0 matches.
> > > > > clang-query> match varDecl(hasType(asString("foo")))
> > > > > 0 matches.
> > > > > clang-query> match varDecl(hasType(asString("bar")))
> > > > > 
> > > > > Match #1:
> > > > > 
> > > > > E:\Desktop\t.cpp:4:1: note: "root" binds here
> > > > > bar i;
> > > > > ^~~~~
> > > > > 1 match.
> > > > > 
> > > > > So hasType() looks at what the immediate type is for the declaration (which we document, yay us!). Based on that, I don't think hasUnderlyingType() makes sense -- you should modify hasType() to work on a TypedefNameDecl (not just a TypedefDecl!) so that it looks at the immediate type of the type definition. I would expect your tests then to result in:
> > > > > ```
> > > > > 1: typedef void (fn)(void);
> > > > > 2: typedef fn foo;
> > > > > 3: typedef int bar;
> > > > > 4: typedef int (f);
> > > > > 5: typedef int (fn2)(int);
> > > > > clang-query> match typedefDecl(hasType(asString("int")))
> > > > > 
> > > > > Match #1:
> > > > > 
> > > > > /tmp/a.cpp:3:1: note: "root" binds here
> > > > > typedef int bar;
> > > > > ^~~~~~~~~~~~~~~
> > > > > 
> > > > > Match #2:
> > > > > 
> > > > > /tmp/a.cpp:4:1: note: "root" binds here
> > > > > typedef int (f);
> > > > > ^~~~~~~~~~~~~~~
> > > > > 2 matches.
> > > > > clang-query> match typedefDecl(hasType(typedefType()))
> > > > > 
> > > > > Match #1:
> > > > > 
> > > > > /tmp/a.cpp:2:1: note: "root" binds here
> > > > > typedef fn foo;
> > > > > ^~~~~~~~~~~~~~
> > > > > 1 match.
> > > > > clang-query> match typedefDecl(hasType(parenType()))
> > > > > 
> > > > > Match #1:
> > > > > 
> > > > > /tmp/a.cpp:1:1: note: "root" binds here
> > > > > typedef void (fn)(void);
> > > > > ^~~~~~~~~~~~~~~~~~~~~~~
> > > > > 
> > > > > Match #2:
> > > > > 
> > > > > /tmp/a.cpp:4:1: note: "root" binds here
> > > > > typedef int (f);
> > > > > ^~~~~~~~~~~~~~~
> > > > > 
> > > > > Match #3:
> > > > > 
> > > > > /tmp/a.cpp:5:1: note: "root" binds here
> > > > > typedef int (fn2)(int);
> > > > > ^~~~~~~~~~~~~~~~~~~~~~
> > > > > 3 matches.
> > > > > clang-query> match typedefDecl(hasType(parenType(innerType(functionType()))))
> > > > > 
> > > > > Match #1:
> > > > > 
> > > > > /tmp/a.cpp:1:1: note: "root" binds here
> > > > > typedef void (fn)(void);
> > > > > ^~~~~~~~~~~~~~~~~~~~~~~
> > > > > 
> > > > > Match #2:
> > > > > 
> > > > > /tmp/a.cpp:5:1: note: "root" binds here
> > > > > typedef int (fn2)(int);
> > > > > ^~~~~~~~~~~~~~~~~~~~~~
> > > > > 2 matches.
> > > > > ```
> > > > > The end results are the same, so this is just changing the way the information is surfaced to the user that is logically consistent. ValueDecls have an immediate type, and so do TypedefDecls. By using TypedefNameDecl instead of TypedefDecl, this also covers the case where hasType() is useful for an alias-declaration. (We don't expose the matcher for that yet, but it seems quite reasonable to add in the future, and it would be nice for hasType to automatically work with that.)
> > > > > 
> > > > > You can implement this with a helper function to handle abstracting away the call to getType() vs getUnderlyingType(), then updating the hasType() matchers to use it. Something like:
> > > > > ```
> > > > > template <typename Ty>
> > > > > struct UnderlyingTypeGetter {
> > > > >   static QualType get(const Ty &Node) {
> > > > >     return Node.getType();
> > > > >   }
> > > > > };
> > > > > 
> > > > > template <>
> > > > > QualType UnderlyingTypeGetter<TypedefNameDecl>::get(const TypedefNameDecl &Node) {
> > > > >   return Node.getUnderlyingType();
> > > > > }
> > > > > ```
> > > > > (Somewhere in ASTMatchersInternal.h most likely.)
> > > > > 
> > > > When I try to extend `hasType` to work on `TypedefDecl`, I get this error:
> > > > 
> > > > ```
> > > > error: static assertion failed: right polymorphic conversion
> > > >      static_assert(TypeListContainsSuperOf<ReturnTypes, T>::value,
> > > > ```
> > > > 
> > > > ...because `TypedefDecl` is derived from `NamedDecl` and the existing definition for `hasType` looks like this:
> > > > 
> > > > ```
> > > > AST_POLYMORPHIC_MATCHER_P_OVERLOAD(hasType,
> > > >                                    AST_POLYMORPHIC_SUPPORTED_TYPES(Expr,
> > > >                                                                    ValueDecl),
> > > >                                    internal::Matcher<Decl>, InnerMatcher, 1) {
> > > >   return qualType(hasDeclaration(InnerMatcher))
> > > >       .matches(Node.getType(), Finder, Builder);
> > > > }
> > > > ```
> > > > 
> > > > So I'll need some guidance on how to extend `hasType` to work for `TypedefNamedDecl` nodes.  I don't understand exactly what all these nasty macros do.  So far, I've simply made changes by imitation, but my approach didn't work this time.
> > > This ({F1302460}) does all of what you need (sans documentation, testing, etc). 
> > > 
> > > (File should be attached, but if you need me to send it to you via email, I can do so -- I've never tried this with Phab before.)
> > What you had was very similar to what I attempted.  You wrote:
> > 
> > ```AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, TypdefNameDecl, ValueDecl)```
> > 
> > and I wrote
> > 
> > ```AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, ValueDecl, TypedefNameDecl)```
> > 
> > so apparently the derivation relations between the arguments to this macro are order dependent?
> I was being OCD and alphabetizing, but I think you may be right that order matters. @sbenza, may know more (if it's purposeful, we should document it).
Order should not matter.
We just iterate over the list to see if there is a matching type.
I could not reproduce the compiler error.


http://reviews.llvm.org/D8149