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

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:02

John Keeping [off-list ref] 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 <redacted>
---
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.
-		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?

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

From: John Keeping <hidden>
Date: 2016-06-15 22:56:02

On Sun, Feb 03, 2013 at 11:58:15AM -0800, Junio C Hamano wrote:
John Keeping [off-list ref] writes:
quoted
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 <redacted>
---
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
quoted
-		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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help