LegalizeAdulthood added inline comments.
================
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:
> LegalizeAdulthood wrote:
> > LegalizeAdulthood wrote:
> > > aaron.ballman wrote:
> > > > 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;
> > > > ```
> > > This all came up because of the work I was doing on clang-tidy to
> > > implement the modernize-redundant-void-arg check. There wasn't a way to
> > > narrow typedefDecl nodes based on the actual type for which a synonym was
> > > being introduced. In that situation, suppose we have something like this:
> > >
> > > ```
> > > typedef void (fnReturningVoid)(void);
> > > typedef foo fnReturningVoid;
> > > ```
> > >
> > > I would want to match the first typedef because that is the one my check
> > > needs to modify. I don't want to match the second typedef because even
> > > though it has the same ultimate type as the first, it doesn't contain the
> > > redundant void argument tokens that need to be fixed up.
> > >
> > > So for my needs in this scenario, the existing matcher does **exactly**
> > > what I need, namely match the first typedef, but not the second. It
> > > turns out that on a typedefDecl node you call the member function
> > > `getUnderlyingType()` in order to match the first typedef, so that is
> > > what I have called the matcher.
> > >
> > > If the phrase "underlying type" in common clang parlance means the
> > > ultimate desugaring of a type down to it's most basic form with all
> > > intervening typedefs removed, then it seems that not only is my matcher
> > > misnamed, but also this member function is misnamed. Because that's not
> > > what this member function is returning for the typedef-of-a-typedef
> > > situation (as the earlier failed test fragment discussed).
> > >
> > > Am I making sense?
> > Oops, that code snippet should have been:
> >
> > ```
> > typedef void (fnReturningVoid)(void);
> > typedef fnReturningVoid foo;
> > ```
> >
> > In other words, `foo` was just anohter synonym for `fnReturningVoid`.
> OK, so I experimented with clang-query a little bit and here's where this
> changeset gets us:
>
> ```
> 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(hasUnderlyingType(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(hasUnderlyingType(typedefType()))
>
> Match #1:
>
> /tmp/a.cpp:2:1: note: "root" binds here
> typedef fn foo;
> ^~~~~~~~~~~~~~
> 1 match.
> ```
>
> ```
> clang-query> match typedefDecl(hasUnderlyingType(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(hasUnderlyingType(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.
> ```
>
> To me this means that the matcher is working as desired. If
> `hasUnderlyingType` squished out all the stuff in the middle, then you
> wouldn't be able to tell the difference between line 1 or line 2. To me it
> seems that the matcher should do just **one** level of narrowing on a
> typedefDecl. If you want to throw all the intervening sugaring away so that
> line 2 and line 1 match `functionType()`, then I think that is a distinct
> matcher, along the lines of existing matchers like `hasDescendent` that
> traverse multiple relationships to find a match. There may even be a way to
> use existing matchers like `has` in order to do this arbitrarily deep
> traversal in order to find a matching type no matter how many layers of
> typedef aliases exist between an identifier and the real type.
>
> Going back to my original motivation, it allows me to match typedef aliases
> for functions taking zero arguments:
>
> ```
> clang-query> match typedefDecl(hasUnderlyingType(parenType(
> innerType(functionType( functionProtoType(parameterCountIs(0)) )) )))
>
> Match #1:
>
> /tmp/a.cpp:1:1: note: "root" binds here
> typedef void (fn)(void);
> ^~~~~~~~~~~~~~~~~~~~~~~
> 1 match.
> ```
Looks like that last one could be simplified slightly:
```
clang-query> match typedefDecl(hasUnderlyingType( parenType(innerType(
functionProtoType(parameterCountIs(0)) )) ))
Match #1:
/tmp/a.cpp:1:1: note: "root" binds here
typedef void (fn)(void);
^~~~~~~~~~~~~~~~~~~~~~~
1 match.
```
http://reviews.llvm.org/D8149
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
