fixes clang pr7161

[chandlerc, 2010-05-20 05:36:55]

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?

[wan, 2010-05-20 06:13:37]

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.

[chandlerc, 2010-05-21 10:32:45]

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.

[wan, 2010-05-21 16:19:50]

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