Re: [PATCH 2/3] combine-diff: suppress a clang warning

From: John Keeping <john@keeping.me.uk>
Date: Feb 3, 2013, 20:31 UTC
Message-ID: <20130203203150.GU1342@serenity.lan>
In-Reply-To: <7vwqup890o.fsf@alter.siamese.dyndns.org>

On Sun, Feb 03, 2013 at 11:58:15AM -0800, Junio C Hamano wrote:

Show 28 quoted lines

> John Keeping <john@keeping.me.uk> writes:
> 
> > When compiling combine-diff.c, clang 3.2 says:
> >
> >     combine-diff.c:1006:19: warning: adding 'int' to a string does not
> > 	    append to the string [-Wstring-plus-int]
> > 		prefix = COLONS + offset;
> > 			 ~~~~~~~^~~~~~~~
> >     combine-diff.c:1006:19: note: use array indexing to silence this warning
> > 		prefix = COLONS + offset;
> > 				^
> > 			 &      [       ]
> >
> > Suppress this by making the suggested change.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> 
> This was not lost in the noise.
> 
> I thought that this wasn't a serious patch, but your attempt to
> demonstrate to others why patches trying to squelch clang warnings
> are not necessarily a good thing to do.
>
> Who is that compiler trying to help with such a warning message?
> After all, we are writing in C, and clang is supposed to be a C
> compiler.  And adding integer to a pointer to (const) char is a
> straight-forward way to look at the trailing part of a given string.

A quick search turned up the original thread where this feature was added to Clang [1]. It seems that it does find genuine bugs where people try to log values by doing:

    log("failed to handle error: " + errno);

[1] http://thread.gmane.org/gmane.comp.compilers.clang.scm/47203

Show 5 quoted lines

> > -		prefix = COLONS + offset;
> > +		prefix = &COLONS[offset];
> 
> In other words, both are perfectly valid C.  Why should we make it
> less readable to avoid a stupid compiler warning?

Are you happy to change COLONS to a const char[] instead of a #define? That also suppresses the warning.

Since Git is warning-free on GCC and so close to being warning-free on recent Clang I think it is worthwhile to fix the remaining two issues which do seem to be intentional diagnostics rather than Clang bugs.

John