Make Git compile warning-free with Clang

From: Antoine Pelisse <[email protected]>

Create a GREP_HEADER_FIELD_MIN so we can check that the field value is sane and silent the clang warning.

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 <[email protected]>

---

Quoted 11 lines · John Keeping

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;
^
& [ ]

Quoted 4 lines

Suppress this by making the suggested change.

Signed-off-by John Keeping <[email protected]>

---

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.

combine-diff.c static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re · line 1006

Quoted 2 lines

combine-diff.c +1 -1

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

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);

Are you happy to change COLONS to a const char[] instead of a #define?

Quoted 5 lines · John Keeping

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);

To be perfectly honest, anybody who writes such a code should be sent back to school before trying to touch out code ever again ;-).

It is not even valid Python, Perl nor Java, I would think.

Quoted 1 line

Are you happy to change COLONS to a const char[] instead of a #define?

Happy? Not really.

It could be a good change for entirely different reason. We will save space if we ever need to use it in multiple places. But the entire "COLONS + offset" thing was a hack we did, knowing that it will break when we end up showing a muiti-way diff for more than 32 blobs.

If we were to be touching that area of code, I'd rather see a change to make it more robust against such a corner case. If it results in squelching misguided clang warnings against programmers who should not be writing in C, that is a nice side effect, but I loathe to see any change whose primary purpose is to squelch pointless warnings.

 combine-diff.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

combine-diff.c +7 -14

index bb1cc96..7f6187f 100644
@@ -982,14 +982,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	free(sline);
 }
 
-#define COLONS "::::::::::::::::::::::::::::::::"
-
 static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)
 {
 	struct diff_options *opt = &rev->diffopt;
-	int i, offset;
-	const char *prefix;
-	int line_termination, inter_name_termination;
+	int line_termination, inter_name_termination, i;
 
 	line_termination = opt->line_termination;
 	inter_name_termination = '\t';

@@ -1000,17 +996,14 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 		show_log(rev);
 
 	if (opt->output_format & DIFF_FORMAT_RAW) {
-		offset = strlen(COLONS) - num_parent;
-		if (offset < 0)
-			offset = 0;
-		prefix = COLONS + offset;
+		/* As many colons as there are parents */
+		for (i = 0; i < num_parent; i++)
+			putchar(':');
 
 		/* Show the modes */
-		for (i = 0; i < num_parent; i++) {
-			printf("%s%06o", prefix, p->parent[i].mode);
-			prefix = " ";
-		}
-		printf("%s%06o", prefix, p->mode);
+		for (i = 0; i < num_parent; i++)
+			printf("%06o ", p->parent[i].mode);
+		printf("%06o", p->mode);
 
 		/* Show sha1's */
 		for (i = 0; i < num_parent; i++)

Quoted 5 lines

If we were to be touching that area of code, I'd rather see a change to make it more robust against such a corner case. If it results in squelching misguided clang warnings against programmers who should not be writing in C, that is a nice side effect, but I loathe to see any change whose primary purpose is to squelch pointless warnings.

This seems like a sensible change.

I generally like to get rid of the pointless warnings so that the useful ones can't hide in the noise. Perhaps "CFLAGS += -Wno-string-plus-int" would be better for this particular warning, but when there's only one bit of code that triggers it, tweaking that seemed simpler.

Ideally it should also have test cases
to show "git diff --cc --raw blob1 blob2...blob$n" for n=4 and n=40 (or any two values clearly below and above the old hardcoded limit) behave sensibly, exposing the old breakage, which I'll leave as a LHF (low-hanging-fruit). Hint, hint...

Signed-off-by John Keeping <[email protected]>

---
...

t/t4038-diff-combined.sh +0 -0

index 40277c7..a0701bc 100755
@@ -89,4 +89,33 @@ test_expect_success 'diagnose truncated file' '
 	grep "diff --cc file" out

I generally like to get rid of the pointless warnings so that the useful ones can't hide in the noise. Perhaps "CFLAGS += -Wno-string-plus-int" would be better for this particular warning, but when there's only one bit of code that triggers it, tweaking that seemed simpler.

Quoted 4 lines · John Keeping

I generally like to get rid of the pointless warnings so that the useful ones can't hide in the noise. Perhaps "CFLAGS += -Wno-string-plus-int" would be better for this particular warning, but when there's only one bit of code that triggers it, tweaking that seemed simpler.

An even better approach would be to file a bug against clang ... it really is a very ill-considered warning -- PTR + OFFS is not just valid C, it's _idiomatic_ in C for getting interior pointers into arrays -- and such a warning should never be enabled by default, or by any standard warning options.

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 <[email protected]>

---
 combine-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

combine-diff.c +0 -0

index bb1cc96..dba4748 100644

Quoted 29 lines · John Keeping

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 <[email protected]>

---
 combine-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

combine-diff.c +1 -1

index bb1cc96..dba4748 100644
@@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
                offset = strlen(COLONS) - num_parent;
                if (offset < 0)
                        offset = 0;
-               prefix = COLONS + offset;
+               prefix = &COLONS[offset];
 

combine-diff.c static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re · line 1009

Quoted 2 lines

combine-diff.c +0 -0

                /* Show the modes */
                for (i = 0; i < num_parent; i++) {

Hmm, does

               prefix = (const char *) COLONS + offset;

suppress the warning?

The first two patches here were sent to the list before but seem to have got lost in the noise [1][2]. The final one is new but was prompted by discussion in the same thread.

builtin/apply.c +3 -2

index 6c11e8b..4745e75 100644
@@ -1041,15 +1041,17 @@ static int gitdiff_renamedst(const char *line, struct patch *patch)
 
 static int gitdiff_similarity(const char *line, struct patch *patch)
 {
-	if ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)
-		patch->score = 0;
+	unsigned long val = strtoul(line, NULL, 10);
+	if (val <= 100)
+		patch->score = val;
 	return 0;
 }