|
|
Patch Set 1 #
Total comments: 10
Patch Set 2 : snapshot 2 #Patch Set 3 : snapshot 3 #Total messages: 4
|
chandlerc
A bit spotty I'm afraid, I'm really tired. I'll try and give this a more ...
|
15 years, 7 months ago (2010年05月20日 05:36:55 UTC) #1 |
A bit spotty I'm afraid, I'm really tired. I'll try and give this a more thorough review tomorrow afternoon, and hopefully get it committed then. http://codereview.appspot.com/1220045/diff/1/2 File include/clang/AST/RecursiveASTVisitor.h (right): http://codereview.appspot.com/1220045/diff/1/2#newcode39 include/clang/AST/RecursiveASTVisitor.h:39: // this class directly - use RecursiveASTVisitor instead. What's the motivation of two classes? I'm not seeing what it buys us here, but that may just be because I've not tried to use it as much. Maybe adding this to the comment here would help. http://codereview.appspot.com/1220045/diff/1/2#newcode682 include/clang/AST/RecursiveASTVisitor.h:682: /** Per IRC, /// comments seem the way to go. http://codereview.appspot.com/1220045/diff/1/2#newcode693 include/clang/AST/RecursiveASTVisitor.h:693: * Base::Visit* is called. It would make more sense (to me) to return 'false' for don't keep recursing through the AST, and 'true' for do keep going. Then the implementors of this visitor needn't know anything about the base class calls.= http://codereview.appspot.com/1220045/diff/1/2#newcode745 include/clang/AST/RecursiveASTVisitor.h:745: DEFINE_VISIT(FunctionProtoType, F, { Exception specifiers?
Please take a look at snapshot 3. Thanks! http://codereview.appspot.com/1220045/diff/1/2 File include/clang/AST/RecursiveASTVisitor.h (right): http://codereview.appspot.com/1220045/diff/1/2#newcode39 include/clang/AST/RecursiveASTVisitor.h:39: // this class directly - use RecursiveASTVisitor instead. On 2010年05月20日 05:36:55, chandlerc wrote: > What's the motivation of two classes? I'm not seeing what it buys us here, but > that may just be because I've not tried to use it as much. Maybe adding this to > the comment here would help. Comment added. http://codereview.appspot.com/1220045/diff/1/2#newcode682 include/clang/AST/RecursiveASTVisitor.h:682: /** On 2010年05月20日 05:36:55, chandlerc wrote: > Per IRC, /// comments seem the way to go. Done. http://codereview.appspot.com/1220045/diff/1/2#newcode693 include/clang/AST/RecursiveASTVisitor.h:693: * Base::Visit* is called. On 2010年05月20日 05:36:55, chandlerc wrote: > It would make more sense (to me) to return 'false' for don't keep recursing > through the AST, and 'true' for do keep going. I agree. Unfortunately this wasn't my decision to make. It's the existing behavior. http://codereview.appspot.com/1220045/diff/1/2#newcode745 include/clang/AST/RecursiveASTVisitor.h:745: DEFINE_VISIT(FunctionProtoType, F, { On 2010年05月20日 05:36:55, chandlerc wrote: > Exception specifiers? Done.
I've committed this upstream in r104315. I also tried to encourage subsequent review, I'd like to make sure we get this API really well fleshed out as its clearly a critical one in the AST. http://codereview.appspot.com/1220045/diff/1/2 File include/clang/AST/RecursiveASTVisitor.h (right): http://codereview.appspot.com/1220045/diff/1/2#newcode39 include/clang/AST/RecursiveASTVisitor.h:39: // this class directly - use RecursiveASTVisitor instead. On 2010年05月20日 06:13:37, wan wrote: > On 2010年05月20日 05:36:55, chandlerc wrote: > > What's the motivation of two classes? I'm not seeing what it buys us here, but > > that may just be because I've not tried to use it as much. Maybe adding this > to > > the comment here would help. > > Comment added. Thanks. No clue why I didn't get this the first time around, not sure it really needs this much commenting, but it can't hurt. =D http://codereview.appspot.com/1220045/diff/1/2#newcode693 include/clang/AST/RecursiveASTVisitor.h:693: * Base::Visit* is called. On 2010年05月20日 06:13:37, wan wrote: > On 2010年05月20日 05:36:55, chandlerc wrote: > > It would make more sense (to me) to return 'false' for don't keep recursing > > through the AST, and 'true' for do keep going. > > I agree. Unfortunately this wasn't my decision to make. It's the existing > behavior. We can always change that behavior. ;] Maybe ask folks on IRC sometime, Nick even assumed that it *did* work this way for a while. Either way, this patch is independent of that change.
On Fri, May 21, 2010 at 3:32 AM, <chandlerc@gmail.com> wrote: > I've committed this upstream in r104315. I also tried to encourage > subsequent review, I'd like to make sure we get this API really well > fleshed out as its clearly a critical one in the AST. Thanks! > http://codereview.appspot.com/1220045/diff/1/2 > File include/clang/AST/RecursiveASTVisitor.h (right): > > http://codereview.appspot.com/1220045/diff/1/2#newcode39 > include/clang/AST/RecursiveASTVisitor.h:39: // this class directly - use > RecursiveASTVisitor instead. > On 2010年05月20日 06:13:37, wan wrote: >> >> On 2010年05月20日 05:36:55, chandlerc wrote: >> > What's the motivation of two classes? I'm not seeing what it buys us > > here, but >> >> > that may just be because I've not tried to use it as much. Maybe > > adding this >> >> to >> > the comment here would help. > >> Comment added. > > Thanks. No clue why I didn't get this the first time around, not sure it > really needs this much commenting, but it can't hurt. =D > > http://codereview.appspot.com/1220045/diff/1/2#newcode693 > include/clang/AST/RecursiveASTVisitor.h:693: * Base::Visit* is called. > On 2010年05月20日 06:13:37, wan wrote: >> >> On 2010年05月20日 05:36:55, chandlerc wrote: >> > It would make more sense (to me) to return 'false' for don't keep > > recursing >> >> > through the AST, and 'true' for do keep going. > >> I agree. Unfortunately this wasn't my decision to make. It's the > > existing >> >> behavior. > > We can always change that behavior. ;] Maybe ask folks on IRC sometime, > Nick even assumed that it *did* work this way for a while. Either way, > this patch is independent of that change. Yes, we can change the behavior if we are willing to break existing clients. I don't feel strongly about the behavior, so I'm not sure if it's worth the disruption. -- Zhanyong