RecursiveASTVisitor with TypeLoc traversal

Add basic TypeLoc traversal to RecursiveASTVisitor. Also moves existing traversals that have easy access to TypeLoc's to using that alternate traversal. The default visitors just call back into the Type visitors so there should be no breakage of compatibility with client code while we sort out the missing pieces of TypeLocs. The implementation falls back to Type traversal when necessary.

[csilvers, 2010-07-04 15:50:42]

It's a bit difficult for me to do diffs from home, but I was so excited by this
patch I thought I'd take a preliminary gander. :-)  Comments inline.

craig

http://codereview.appspot.com/1700049/diff/2001/3001
File include/clang/AST/RecursiveASTVisitor.h (right):

http://codereview.appspot.com/1700049/diff/2001/3001#newcode310
include/clang/AST/RecursiveASTVisitor.h:310: // methods call into the equivalent
Visit*() method for Type classes.
Is it correct that the only method that client classes should be subclassing is
VisitTypeLoc?  I'm not sure I understand how VisitQualifiedTypeLoc/etc relate. 
I'm also not sure I totally understand how I would change a client to use
TypeLoc's when available and fall back onto types if not.  I guess I'd have to
write a visitor for each of them, since some parts of RAV.h might only have
access to the type, and others might have the typeloc?

http://codereview.appspot.com/1700049/diff/2001/3001#newcode448
include/clang/AST/RecursiveASTVisitor.h:448: return
getDerived().Traverse##CLASS##TypeLoc(*cast<CLASS##TypeLoc>(&TL));
Can you not just use DISPATCH here?

Also, I notice you use cast<> where similar parts of the code use dyn_cast<>
(DISPATCH does, I think).  Should we be consistent?  I don't know which is
better.

http://codereview.appspot.com/1700049/diff/2001/3001#newcode538
include/clang/AST/RecursiveASTVisitor.h:538: // FIXME: we drop the template name
location. Do we have a use for it?
I think it's good to have -- any reason not to?  In theory, we should have
TypeLoc versions of all these traversal routines here, including
NestedNameSpace.

[nlewycky, 2010-07-04 17:17:36]

Thanks for the weekend review!

http://codereview.appspot.com/1700049/diff/2001/3001
File include/clang/AST/RecursiveASTVisitor.h (right):

http://codereview.appspot.com/1700049/diff/2001/3001#newcode310
include/clang/AST/RecursiveASTVisitor.h:310: // methods call into the equivalent
Visit*() method for Type classes.
On 2010/07/04 15:50:43, csilvers wrote:
> Is it correct that the only method that client classes should be subclassing
is
> VisitTypeLoc?

Eventually. Right now you'll miss some things because we traverse everything as
Type but merely most things as TypeLoc. After this patch, subclassing Visit*Type
should have the exact same behaviour it currently does, while Visit*TypeLoc will
be available for people who want it but is incomplete.

> I'm not sure I understand how VisitQualifiedTypeLoc/etc relate.

The TypeLoc hierarchy is the same as the Type hierarchy except that:
  a) QualType isn't part of the Type hierarchy while QualifiedTypeLoc is derived
from TypeLoc. Nothing derives from QualifiedTypeLoc, it provides a getter to
return the UnqualTypeLoc.
  b) The only two direct subclasses of TypeLoc are QualifiedTypeLoc and
UnqualTypeLoc. TypeLoc's copy of the Type hierarchy is rooted at UnqualTypeLoc
instead of directly on TypeLoc.

> I'm also not sure I totally understand how I would change a client to use
> TypeLoc's when available and fall back onto types if not.  I guess I'd have to
> write a visitor for each of them, since some parts of RAV.h might only have
> access to the type, and others might have the typeloc?

Yes. Ultimately I'd like to get rid of Type traversal entirely, but this lets me
get started testing TypeLoc support without breaking existing users.

http://codereview.appspot.com/1700049/diff/2001/3001#newcode448
include/clang/AST/RecursiveASTVisitor.h:448: return
getDerived().Traverse##CLASS##TypeLoc(*cast<CLASS##TypeLoc>(&TL));
On 2010/07/04 15:50:43, csilvers wrote:
> Can you not just use DISPATCH here?

Nope! Note that DISPATCH casts to CLASS* while TypeLocs are passed by copy, not
pointer. I could redefine DISPATCH if you really want, I suppose.

> Also, I notice you use cast<> where similar parts of the code use dyn_cast<>
> (DISPATCH does, I think).  Should we be consistent?  I don't know which is
> better.

dyn_cast<> is used when you're not sure what the actual type is. It returns NULL
if the object isn't of the target type. In this case, we've got a switch
statement on the concrete type so we can safely just cast<>.

I did a quick audit of the users of dyn_cast<> in this file and they all look
correct to me.

http://codereview.appspot.com/1700049/diff/2001/3001#newcode538
include/clang/AST/RecursiveASTVisitor.h:538: // FIXME: we drop the template name
location. Do we have a use for it?
On 2010/07/04 15:50:43, csilvers wrote:
> I think it's good to have -- any reason not to?  In theory, we should have
> TypeLoc versions of all these traversal routines here, including
> NestedNameSpace.

Right, I'll update the comment to indicate that we want to but there's no
matching TypeLoc concept for us to use.

[nlewycky, 2010-07-04 21:02:20]

Also fix TypeLoc FIXME in TemplateTemplateParmDecl

[csilvers, 2010-07-04 21:18:41]

I'm obviously not the best person to review this code, since I'm only starting
to get familiar with clang, but it looks good to me.  I'm not sure I totally
understand how to use this yet, but I'm sure we can figure it out -- I'm happy
for my project to be a guinea pig.

craig

http://codereview.appspot.com/1700049/diff/2001/3001
File include/clang/AST/RecursiveASTVisitor.h (right):

http://codereview.appspot.com/1700049/diff/2001/3001#newcode1108
include/clang/AST/RecursiveASTVisitor.h:1108:
TRY_TO(TraverseType(D->getDefaultArgument()));
You can change this to TraverseTypeInfo if you use getDefaultArgumentInfo(), no?

Are there other existing calls to TraverseType you can change to
TraverseTypeLoc?

[nlewycky, 2010-07-04 21:25:56]

http://codereview.appspot.com/1700049/diff/2001/3001
File include/clang/AST/RecursiveASTVisitor.h (right):

http://codereview.appspot.com/1700049/diff/2001/3001#newcode1108
include/clang/AST/RecursiveASTVisitor.h:1108:
TRY_TO(TraverseType(D->getDefaultArgument()));
On 2010/07/04 21:18:41, csilvers wrote:
> You can change this to TraverseTypeInfo if you use getDefaultArgumentInfo(),
no?
> 
> Are there other existing calls to TraverseType you can change to
> TraverseTypeLoc?

Great catch! I meant to change all of them but missed TemplateTypeParmDecl here
and TemplateTemplateParmDecl above. I think that's all of them (without adding
new ones) and we can fix any others with later patches.

[DougGregor, 2010-07-06 17:51:45]

Looks good, please go ahead and commit.

http://codereview.appspot.com/1700049/diff/9001/10001
File include/clang/AST/RecursiveASTVisitor.h (right):

http://codereview.appspot.com/1700049/diff/9001/10001#newcode829
include/clang/AST/RecursiveASTVisitor.h:829: // Note that we have the
ParmVarDecl's here. Do we want to use them?
The lack of source location information for exception specifications is this
bug:

  http://llvm.org/bugs/show_bug.cgi?id=5719

And IMO, yes, we do want to walk the ParmVarDecls here.