From: David Michael Barr <davidbarr@google.com>
Date: Thu, 31 May 2012 11:17:03 GMT
Subject: Re: [PATCH 5/7] vcs-svn: fix cppcheck warning
Message-ID: <CAFfmPPPZpH7WMLtE9YVOfzO2pjU8Scudg4hVp-ot+SAFxKLs3Q@mail.gmail.com>
In-Reply-To: <20120524142058.GA3732@burratino>

On Fri, May 25, 2012 at 12:20 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> First, thanks much for this series!  I especially like patch #4.
>
> David Barr wrote:
>
>> [vcs-svn/fast_export.c:211]: (warning) Using sizeof with a numeric constant
>> as function argument might not be what you intended.
> [...]
>> -             assert(!signed_add_overflows(preimage.max_off, 1));
>> +             assert(!signed_add_overflows(preimage.max_off, (off_t) 1));
>
> I think this is due to a typo in the compat-util.h compatibility layer
> you are using.  In git.git there is
>
>        #define signed_add_overflows(a, b) \
>            ((b) > maximum_signed_value_of_type(a) - (a))
>
> so the sizeof() operator is applied to preimage.max_off and all is well.
> By contrast, in svn-dump-fast-export.git it says
>
>        #define signed_add_overflows(a, b) \
>                ((a) > maximum_signed_value_of_type(b) - (b))
>
> Though the comment describing signed_add_overflows() does say that "a"
> and "b" should have the same type, I like being able to ask if
> signed_add_overflows(n, 1) in a non-fussy way that does not introduce
> subtle bugs when the type of "n" changes, so I'd prefer not to take
> this patch.

Thank you for the review. I will queue the typo fix upstream and drop
this patch.

--
David Barr