Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

> 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.