Re: [PATCH 2/3] combine-diff: suppress a clang warning
On Thu, Feb 07, 2013 at 01:12:59PM +0900, Miles Bader wrote: > John Keeping writes: > > 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. It doesn't warn of PTR + OFFS, only STRING_LITERAL + OFFS. I agree that it's not a particularly useful warning but it was clearly introduced intentionally and appears to find real bugs [1] so I don't intend to argue about it with the Clang developers. [1] http://article.gmane.org/gmane.comp.compilers.clang.scm/47203 John -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] combine-diff: suppress a clang warning
John Keeping writes: > 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. -miles -- 永日の 澄んだ紺から 永遠へ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] combine-diff: suppress a clang warning
John Keeping writes:
>> 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.
Thanks for a sanity check. 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...
-- >8 --
Subject: [PATCH] combine-diff: lift 32-way limit of combined diff
The "raw" format of combine-diff output is supposed to have as many
colons as there are parents at the beginning, then blob modes for
these parents, and then object names for these parents.
We weren't however prepared to handle a more than 32-way merge and
did not show the correct number of colons in such a case.
Signed-off-by: Junio C Hamano
---
combine-diff.c | 21 +++--
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..7f6187f 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -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++)
--
1.8.1.2.628.geb8a6d5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] combine-diff: suppress a clang warning
On Sun, Feb 03, 2013 at 01:07:48PM -0800, Junio C Hamano wrote:
> John Keeping writes:
>
> > 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 ;-).
Yeah, I can't see that getting through review here :-).
> It is not even valid Python, Perl nor Java, I would think.
It is valid Java, although I can't think of any other languages that let
you do that.
> > 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.
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.
> combine-diff.c | 21 +++--
> 1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index bb1cc96..7f6187f 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -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++)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] combine-diff: suppress a clang warning
John Keeping writes:
> 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.
> 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(-)
diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..7f6187f 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -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++)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] combine-diff: suppress a clang warning
On Sun, Feb 03, 2013 at 11:58:15AM -0800, Junio C Hamano wrote:
> John Keeping 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
> > ---
>
> 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
> > - 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] combine-diff: suppress a clang warning
John Keeping 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 > --- 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] combine-diff: suppress a clang warning
On Mon, Feb 04, 2013 at 02:20:06AM +0800, Tay Ray Chuan wrote:
> On Sun, Feb 3, 2013 at 10:37 PM, John Keeping wrote:
> > 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
> > ---
> > combine-diff.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/combine-diff.c b/combine-diff.c
> > index bb1cc96..dba4748 100644
> > --- a/combine-diff.c
> > +++ b/combine-diff.c
> > @@ -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];
> >
> > /* Show the modes */
> > for (i = 0; i < num_parent; i++) {
>
> Hmm, does
>
>prefix = (const char *) COLONS + offset;
>
> suppress the warning?
It does, but it turns out that the following also suppresses the
warning:
-- >8 --
diff --git a/combine-diff.c b/combine-diff.c
index bb1cc96..a07d329 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -982,7 +982,7 @@ static void show_patch_diff(struct combine_diff_path *elem,
int num_parent,
free(sline);
}
-#define COLONS ""
+static const char COLONS[] = "";
static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct
rev_info *rev)
{
I think that's a nicer change than the original suggestion.
John
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] combine-diff: suppress a clang warning
On Sun, Feb 3, 2013 at 10:37 PM, John Keeping wrote:
> 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
> ---
> combine-diff.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/combine-diff.c b/combine-diff.c
> index bb1cc96..dba4748 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -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];
>
> /* Show the modes */
> for (i = 0; i < num_parent; i++) {
> --
Hmm, does
prefix = (const char *) COLONS + offset;
suppress the warning?
--
Cheers,
Ray Chuan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
