[cfe-commits] Properly visit CXXForRangeStmts in RecursiveASTVisitor
djasper at google.com
Wed Jun 13 00:14:34 PDT 2012
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>
>> > Hi Daniel,
>> > On Tue, Jun 12, 2012 at 10:36 PM, Daniel Jasper <djasper at google.com>
>> >> 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
>> >> currently do.
>> >> If not, we should explicitly visit the loop variable (but not its
>> >> initialization) and the range initialization. Otherwise, we would not
>> >> 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,
>> > 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...
More information about the cfe-commits