On Sat, 26 Sep 2009, Jeff King wrote: > On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote: > > > Here is a cleaned patch. I think these assignments can be removed > > without any problem. > > I don't agree. For example: > > > --- a/builtin-fetch--tool.c > > +++ b/builtin-fetch--tool.c > > @@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp, > > note_len += sprintf(note + note_len, "%s ", kind); > > note_len += sprintf(note + note_len, "'%s' of ", what); > > } > > - note_len += sprintf(note + note_len, "%.*s", remote_len, remote); > > + sprintf(note + note_len, "%.*s", remote_len, remote); > > This is a very particular C idiom: you are building a string over > several statements using a function that adds to the string and tells > you how much it added. The implicit invariant of the note_len variable > is that it _always_ contains the current length, so each statement uses > it as input and pushes it forward on output. > > Any experienced C programmer should look at that and be able to see > exactly what's going on. And people adding more lines don't need to > munge the existing lines; the invariant property of note_len means they > just need to add more, similar lines. > > But your patch destroys that invariant. It makes it harder to see what's > going on, because it breaks the idiom. And it makes it more likely for > somebody adding a line further on to make a mistake (and certainly it > makes their patch harder to read and review, as they have to munge > unrelated lines). > > So no, while there is no code _now_ that is relying on the invariant > being kept after the last statement (which is what the static analyzer > is finding out), the point is not for the compiler to realize that, but > for human programmers to see it. And the compiler (at least gcc) is indeed smart enough to realize that nothing uses the result from the last statement, and does optimize away the code associated to it already. So this patch is unlikely to change anything to the compiled result. Nicolas -- 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