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:
> 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`.
http://reviews.llvm.org/D8149
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
