Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jun 20, 2014, 08:59 UTC
- Message-ID
- <CAPig+cS6MoU0zDxY9ijMZmVGH04s_7RHLNoW8UVhQ9ff1OkiDA@mail.gmail.com>
- In-Reply-To
- <20140620054549.GA4623@sigill.intra.peff.net>
On Fri, Jun 20, 2014 at 1:45 AM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> 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.
Show 6 quoted lines
> 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.