Changed method name and comment and committed as r158395.<br><br><div class="gmail_quote">On Wed, Jun 13, 2012 at 8:02 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div class="h5">On Tue, Jun 12, 2012 at 10:57 PM, James Dennett <span dir="ltr"><<a href="mailto:jdennett@google.com" target="_blank">jdennett@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div>On Tue, Jun 12, 2012 at 10:48 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
> Hi Daniel,<br>
><br>
><br>
> On Tue, Jun 12, 2012 at 10:36 PM, Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>> wrote:<br>
>><br>
>> Please find the attached patch, which changes the RecursiveASTVisitor<br>
>> behavior for CXXForRangeStmts, for review.<br>
>><br>
>> Depending on whether a RecursiveASTVisitor should visit implicit<br>
>> declarations, we need to do different things when visiting a range-based for<br>
>> loop.<br>
>><br>
>> If visiting implicit declarations, we can simply visit all children as we<br>
>> currently do.<br>
>><br>
>> If not, we should explicitly visit the loop variable (but not its implicit<br>
>> initialization) and the range initialization. Otherwise, we would not visit<br>
>> the range initialization at all (although it is explicit) because it is part<br>
>> of an implicit VarDecl.<br>
><br>
><br>
> The patch looks great, but please update the comment for<br>
> shouldVisitImplicitDeclarations to indicate that it also controls whether we<br>
> visit implicit expressions in some cases. (I'm wary of renaming it, because<br>
> that might silently break external RAV users...)<br>
<br>
</div></div>While breakage is possible (and worthy of caution),<br>
shouldVisitImplicitDeclarations() was added just a week ago.  As one<br>
of its users, I'd happily pay the price of updating my code if it<br>
meant avoiding a misleading function name.  (One of those eternal<br>
tradeoffs: the more specific a name is, the more resistant it is to<br>
future changes.)<br>
<br>
That said, I don't have a good suggestion for a name;<br>
shouldVisitImplicitCode, maybe.  By the time we cover declarations and<br>
expressions, there's not a whole lot left (well, other statements, I<br>
guess, but why make the name so specific that it rules that out).</blockquote><div><br></div></div></div><div>OK, shouldVisitImplicitCode sounds reasonable.</div></div>
</blockquote></div><br>