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