On Sun, Feb 03, 2013 at 01:07:48PM -0800, Junio C Hamano wrote:
> John Keeping <j...@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.

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

Reply via email to