From: David Michael Barr <davidbarr@google.com>
Date: Thu, 31 May 2012 11:34:59 GMT
Subject: Re: [PATCH 7/7] vcs-svn: fix clang-analyzer warning
Message-ID: <CAFfmPPPmnS4f4+JVhLFqcNM+taj4VO6ZVMv8RCoEiQqKHGy_1g@mail.gmail.com>
In-Reply-To: <20120524143337.GB3732@burratino>

On Fri, May 25, 2012 at 12:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> David Barr wrote:
>
>> vcs-svn/svndiff.c:278:3: warning: expression result unused [-Wunused-value]
>>                 error("invalid delta: incorrect postimage length");
>>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from vcs-svn/svndiff.c:6:
>> vcs-svn/compat-util.h:18:61: note: instantiated from:
>> #define error(...) (fprintf(stderr, "error: " __VA_ARGS__), -1)
>>                                                             ^~
>
> Yuck.  Would you be ok with an inline variadic function?
>
>  static inline int error(const char *fmt, ...)
>  {
>        va_list ap;
>
>        fprintf(stderr, "error: ");
>
>        va_start(ap, fmt);
>        vfprintf(stderr, fmt, ap)
>        va_end(ap);
>
>        fprintf(stderr, "\n");
>
>        return -1;
>  }
>
> The error() macro above also seems to leave out a newline.
>
>> --- a/vcs-svn/svndiff.c
>> +++ b/vcs-svn/svndiff.c
>> @@ -258,6 +258,7 @@ static int apply_window_in_core(struct window *ctx)
> [...]
>> @@ -275,16 +276,15 @@ static int apply_one_window(struct line_buffer *delta, off_t *delta_len,
>>       if (apply_window_in_core(&ctx))
>>               goto error_out;
>>       if (ctx.out.len != out_len) {
>> -             error("invalid delta: incorrect postimage length");
>> +             rv = error("invalid delta: incorrect postimage length");
>>               goto error_out;
>>       }
>>       if (write_strbuf(&ctx.out, out))
>>               goto error_out;
>> -     window_release(&ctx);
>> -     return 0;
>> +     rv = 0;
>>  error_out:
>>       window_release(&ctx);
>> -     return -1;
>> +     return rv;
>
> That said, if this change is justified by saying that it avoids having
> to repeat the cleanup code, it already looks like a good change.  The
> commit message could mention that the original motivation and a
> side-benefit is to help the standalone version that has a slightly
> crazier definition of error().

I'll rework the commit message and requeue.

--
David Barr.