Re: [PATCH 2/3] combine-diff: suppress a clang warning
- From
John Keeping <john@keeping.me.uk>
- Date
- Feb 3, 2013, 23:15 UTC
- Message-ID
- <20130203231549.GV1342@serenity.lan>
- In-Reply-To
- <7v8v7585sr.fsf@alter.siamese.dyndns.org>
On Sun, Feb 03, 2013 at 01:07:48PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> John Keeping <john@keeping.me.uk> 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.
Show 15 quoted lines
> > 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.
Show 47 quoted lines
> 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++)