[cfe-commits] Properly visit CXXForRangeStmts in RecursiveASTVisitor

[Daniel Jasper]

Changed method name and comment and committed as r158395.

On Wed, Jun 13, 2012 at 8:02 AM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Tue, Jun 12, 2012 at 10:57 PM, James Dennett <jdennett at google.com>wrote:
>
>> 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).
>
>
> OK, shouldVisitImplicitCode sounds reasonable.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120613/f607567a/attachment.html>