Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(7066)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 1607042: change RecursiveASTVisitor to the new design and improve coverage.

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 10 months ago by wan
Modified:
13 years, 10 months ago
Reviewers:
chandlerc, DougGregor
CC:
csilvers
Base URL:
http://llvm.org/svn/llvm-project/cfe/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 66

Patch Set 2 : addresses Doug and Chandler's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+937 lines, -419 lines) Patch
M include/clang/AST/RecursiveASTVisitor.h View 1 10 chunks +919 lines, -401 lines 0 comments Download
M lib/Frontend/BoostConAction.cpp View 2 chunks +4 lines, -4 lines 0 comments Download
M lib/Sema/SemaExpr.cpp View 2 chunks +14 lines, -14 lines 0 comments Download

Messages

Total messages: 3
DougGregor
This looks great! I've made a bunch of comments throughout, but feel free to go ...
13 years, 10 months ago (2010-06-09 00:31:31 UTC) #1
chandlerc
Just a few stylistic comments. Doug hit just about all of the AST stuff I ...
13 years, 10 months ago (2010-06-09 01:31:23 UTC) #2
wan
13 years, 10 months ago (2010-06-09 07:29:56 UTC) #3 
Thanks for the excellent comments, Doug and Chandler!  I think I've addressed
all of them.  Would you please take another look (I've uploaded a new patch to
the same issue)?  Thanks.

http://codereview.appspot.com/1607042/diff/1/2
File include/clang/AST/RecursiveASTVisitor.h (right):

http://codereview.appspot.com/1607042/diff/1/2#newcode33
include/clang/AST/RecursiveASTVisitor.h:33: // FIXME: reverse the meaning of the
bool return value.
On 2010/06/09 01:31:23, chandlerc wrote:
> Maybe attach this FIXME to the class comment where you explain the meaning?

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode49
include/clang/AST/RecursiveASTVisitor.h:49: /// methods (except Traverse*()) for
declaration, type, statement,
On 2010/06/09 01:31:23, chandlerc wrote:
> s/except/but not/? Otherwise, this sentence doesn't parse for me...

Reworded.

http://codereview.appspot.com/1607042/diff/1/2#newcode54
include/clang/AST/RecursiveASTVisitor.h:54: /// Base::Visit* is called.
On 2010/06/09 01:31:23, chandlerc wrote:
> This last sentence no longer seems accurate.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode115
include/clang/AST/RecursiveASTVisitor.h:115: #define STMT(CLASS, PARENT)        
                            \
On 2010/06/09 01:31:23, chandlerc wrote:
> Do we want to braket these defines tightly around the #include of the nodes
inc
> file? I'm a little worried about how much code in this file has STMT defined
for
> it, but not too worried if you think re-defining things will be too annoying.

The .inc file takes care of #undef-ing the macros.  Added a comment to that
effect.

http://codereview.appspot.com/1607042/diff/1/2#newcode209
include/clang/AST/RecursiveASTVisitor.h:209: #define ABSTRACT_TYPE(CLASS, BASE)
On 2010/06/09 01:31:23, chandlerc wrote:
> FYI, unless I missed something, we've still not undef'ed STMT and
ABSTRACT_STMT.

See above.

http://codereview.appspot.com/1607042/diff/1/2#newcode455
include/clang/AST/RecursiveASTVisitor.h:455: // we enforce the following
constraint: only the leaf-node visitors
On 2010/06/09 01:31:23, chandlerc wrote:
> In this paragraph and the next you talk about 'visitors' where I think you
mean
> 'Traverse* methods' or something similar given the new design.

Renamed to "traversal".

http://codereview.appspot.com/1607042/diff/1/2#newcode459
include/clang/AST/RecursiveASTVisitor.h:459: // With this constraint, we can
design every leaf-node visitor like
On 2010/06/09 01:31:23, chandlerc wrote:
> s/like/as/

Got rid of the entire comment block, as now we have described the design in
details at the beginning of the class.

http://codereview.appspot.com/1607042/diff/1/2#newcode465
include/clang/AST/RecursiveASTVisitor.h:465: // For types, we iterate, in the
macro, iterating over the parent.
On 2010/06/09 01:31:23, chandlerc wrote:
> This sentence doesn't parse for me. Is entire paragraph needed given the
> comments above?

Removed.

http://codereview.appspot.com/1607042/diff/1/2#newcode626
include/clang/AST/RecursiveASTVisitor.h:626: // if any, then it iterates over
its children, if any.
On 2010/06/09 01:31:23, chandlerc wrote:
> I think these two paragraphs, rather than being repeated three times, should
go
> somewher at the top of the file describing the strategy of the overall class.

Removed.

http://codereview.appspot.com/1607042/diff/1/2#newcode655
include/clang/AST/RecursiveASTVisitor.h:655: 
On 2010/06/09 00:31:32, DougGregor wrote:
> I think the parameters of a block will be in the decl_begin()/decl_end()
> iteration for the BlockDecl, so you don't need to iterate over them here. If
> not, they should be in the BlockDecl.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode702
include/clang/AST/RecursiveASTVisitor.h:702: 
On 2010/06/09 00:31:32, DougGregor wrote:
> Yes. The anonymous namespace will be represented by a NamespaceDecl inside the
> decl_begin()/decl_end() iteration, so we should not recurse on
> D->getAnonymousNamespace() here.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode707
include/clang/AST/RecursiveASTVisitor.h:707: 
On 2010/06/09 00:31:32, DougGregor wrote:
> We shouldn't traverse the aliased namespace, since it will be defined (and,
> therefore, traversed) somewhere else.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode715
include/clang/AST/RecursiveASTVisitor.h:715: 
On 2010/06/09 00:31:32, DougGregor wrote:
> Same comment as before: the anonymous namespace will show up in
> decl_begin()/decl_end(), so it shouldn't be traversed here.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode782
include/clang/AST/RecursiveASTVisitor.h:782: 
On 2010/06/09 00:31:32, DougGregor wrote:
> This should not iterate over the specializations/partial specializations.
Those
> will show up in other contexts.
> 
> getInstantiatedFromMemberTemplate() is just a link from a template
instantiation
> back to the template from which it was instantiated. It should not be
traversed.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode814
include/clang/AST/RecursiveASTVisitor.h:814: 
On 2010/06/09 00:31:32, DougGregor wrote:
> I don't think you should traverse D->getTypeForDecl(); it's a result of
> declaring the typedef, not something that was written in the source.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode822
include/clang/AST/RecursiveASTVisitor.h:822: 
On 2010/06/09 00:31:32, DougGregor wrote:
> Same comment as above: we shouldn't traverse D->getTypeForDecl().

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode834
include/clang/AST/RecursiveASTVisitor.h:834: 
On 2010/06/09 00:31:32, DougGregor wrote:
> The enumerators will already be traversed by decl_begin()/decl_end(); you
don't
> want to walk them here.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode842
include/clang/AST/RecursiveASTVisitor.h:842: 
On 2010/06/09 00:31:32, DougGregor wrote:
> Don't traverse D->getTypeForDecl().

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode847
include/clang/AST/RecursiveASTVisitor.h:847: 
On 2010/06/09 00:31:32, DougGregor wrote:
> The anonymous struct or union object is the variable or field whose type is
the
> anonymous struct or union. We shouldn't walk "up" the tree like this. While it
> won't actually cause recursion (since we don't visit the Decl behind a
> RecordType), it's not something that is explicitly written in the source.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode867
include/clang/AST/RecursiveASTVisitor.h:867: }
On 2010/06/09 00:31:32, DougGregor wrote:
> I believe that all of the friends are in decl_begin()/decl_end().

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode873
include/clang/AST/RecursiveASTVisitor.h:873: }
On 2010/06/09 00:31:32, DougGregor wrote:
> The conversion functions are *definitely* in decl_begin()/end(), so this will
> double-count.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode885
include/clang/AST/RecursiveASTVisitor.h:885: }
On 2010/06/09 00:31:32, DougGregor wrote:
> We probably want to traverse the TemplateArgumentLoc for explicitly-written
> specializations and instantiations, rather than the computed template
arguments.

Added a FIXME.

http://codereview.appspot.com/1607042/diff/1/2#newcode966
include/clang/AST/RecursiveASTVisitor.h:966: }
On 2010/06/09 00:31:32, DougGregor wrote:
> The function parameters will already be visited by decl_begin()/decl_end().

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode967
include/clang/AST/RecursiveASTVisitor.h:967:
TRY_TO(TraverseType(D->getResultType()));
On 2010/06/09 00:31:32, DougGregor wrote:
> This doesn't seem to be necessary. The result type is part of the
TypeSourceInfo
> for the function declaration.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode1040
include/clang/AST/RecursiveASTVisitor.h:1040: // FIXME: do we need to recurse on
getOriginalType()?
On 2010/06/09 00:31:32, DougGregor wrote:
> Recursing on the TypeSourceInfo is far better that recursing on
> getOriginalType(), since the TypeSourceInfo describes the original type as
> written.

Adjusted the FIXME.

http://codereview.appspot.com/1607042/diff/1/2#newcode1097
include/clang/AST/RecursiveASTVisitor.h:1097: 
On 2010/06/09 00:31:32, DougGregor wrote:
> We'll end up visiting the exception object, which already has this type.
That's
> where S->getCaughtType() actually gets the type information from, so it's
> redundant to visit here.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode1153
include/clang/AST/RecursiveASTVisitor.h:1153: }
On 2010/06/09 00:31:32, DougGregor wrote:
> Perhaps you want to create a TemplateArgumentLoc* traversal helper? There are
> several loops like this.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode1182
include/clang/AST/RecursiveASTVisitor.h:1182: // FIXME: make a type callback for
the cast?
On 2010/06/09 00:31:32, DougGregor wrote:
> IMO, no: the type wasn't written in the source so we shouldn't have a callback
> for it.

Done.

http://codereview.appspot.com/1607042/diff/1/2#newcode1208
include/clang/AST/RecursiveASTVisitor.h:1208: 
On 2010/06/09 00:31:32, DougGregor wrote:
> In all of these cases, I think the answer is "yes". We should recurse on the
> TypeSourceInfo* for the type as is was written.

Done.  I'm traversing the QualType for now.  We'll switch from QualType to
TypeLoc everywhere later.

http://codereview.appspot.com/1607042/diff/1/2#newcode1289
include/clang/AST/RecursiveASTVisitor.h:1289: //    ExplicitCastExpr
On 2010/06/09 00:31:32, DougGregor wrote:
> Definitely worth traversing the TypeSourceInfo* here.

Removed the comment as we are already doing it in the concrete subclasses of
ExplicitCastExpr.

http://codereview.appspot.com/1607042/diff/1/2#newcode1290
include/clang/AST/RecursiveASTVisitor.h:1290: //    ImplicitCastExpr?
On 2010/06/09 00:31:32, DougGregor wrote:
> IMO, no.

Removed the comment.

http://codereview.appspot.com/1607042/diff/1/2#newcode1296
include/clang/AST/RecursiveASTVisitor.h:1296: 
On 2010/06/09 00:31:32, DougGregor wrote:
> Yes. Note that we have an open enhancement request to implement proper
> source-location information for nested-name-specifiers, but we could still
walk
> each of these NestedNameSpecifiers now.

Fixed the comment.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b