[cfe-commits] Properly visit CXXForRangeStmts in RecursiveASTVisitor

[Richard Smith]

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/20120612/62849c26/attachment.html>