Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 20, 2014 at 1:45 AM, Jeff King <peff@xxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]