On Fri, Jun 20, 2014 at 1:45 AM, Jeff King <p...@peff.net> wrote:
> On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote:
>> > - if (starts_with(command_buf.buf, "M "))
>> > - file_change_m(b);
>> > - else if (starts_with(command_buf.buf, "D "))
>> > - file_change_d(b);
>> > - else if (starts_with(command_buf.buf, "R "))
>> > - file_change_cr(b, 1);
>> > - else if (starts_with(command_buf.buf, "C "))
>> > - file_change_cr(b, 0);
>> > - else if (starts_with(command_buf.buf, "N "))
>> > - note_change_n(b, &prev_fanout);
>> > + const char *v;
>> This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in
>> the function.
> Thanks. I reordered the patches before sending, so when this one was
> originally written, there was no "v" at the top-level of the function.
> I think we can just drop this interior one. The point of the short "v"
> is that it can be used as a temporary value for prefix matches, so I
> think we can just reuse the same one.
Agreed. The intended usage of 'v' is clear enough and the code simple
enough that confusion is unlikely.
> I tried compiling with -Wshadow (which I don't usually do), but we're
> not even close to compiling clean there. Some of them are legitimately
> confusing (e.g., try figuring out "end" in parse_rev_note). But others
> look just annoying (e.g., complaining that a local "usage" conflicts
> with the global function). I don't know if we want to put effort into
> being -Wshadow clean or not.
I just happened to notice the shadowing declaration while reading the
patch, but don't feel strongly about existing cases. It makes sense to
clean up confusing cases, such 'end' in parse_rev_note(), when working
on that code (just as with style cleanups), but thus far nobody has
been complaining about existing shadowed variables, so global cleanup
would likely be considered churn.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html