[cfe-commits] Properly visit CXXForRangeStmts in RecursiveASTVisitor

[James Dennett]

On Tue, Jun 12, 2012 at 10:48 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Hi Daniel,
>
>
> On Tue, Jun 12, 2012 at 10:36 PM, Daniel Jasper <djasper at google.com> wrote:
>>
>> Please find the attached patch, which changes the RecursiveASTVisitor
>> behavior for CXXForRangeStmts, for review.
>>
>> Depending on whether a RecursiveASTVisitor should visit implicit
>> declarations, we need to do different things when visiting a range-based for
>> loop.
>>
>> If visiting implicit declarations, we can simply visit all children as we
>> currently do.
>>
>> If not, we should explicitly visit the loop variable (but not its implicit
>> initialization) and the range initialization. Otherwise, we would not visit
>> the range initialization at all (although it is explicit) because it is part
>> of an implicit VarDecl.
>
>
> The patch looks great, but please update the comment for
> shouldVisitImplicitDeclarations to indicate that it also controls whether we
> visit implicit expressions in some cases. (I'm wary of renaming it, because
> that might silently break external RAV users...)

While breakage is possible (and worthy of caution),
shouldVisitImplicitDeclarations() was added just a week ago.  As one
of its users, I'd happily pay the price of updating my code if it
meant avoiding a misleading function name.  (One of those eternal
tradeoffs: the more specific a name is, the more resistant it is to
future changes.)

That said, I don't have a good suggestion for a name;
shouldVisitImplicitCode, maybe.  By the time we cover declarations and
expressions, there's not a whole lot left (well, other statements, I
guess, but why make the name so specific that it rules that out).

-- James