Re: [PATCH 2/7] diffcore-rename: remove unnecessary if-clause

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 10, 2020, 02:03 UTC
Message-ID: <xmqqtusuxvr8.fsf@gitster.c.googlers.com>
In-Reply-To: <X9FLaiuWpYely6es@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 13 quoted lines

> On Sun, Dec 06, 2020 at 02:54:31AM +0000, Elijah Newren via GitGitGadget wrote:
>> From: Elijah Newren <newren@gmail.com>
>>
>> diffcore-rename had two different checks of the form
>>
>>     if ((a < limit || b < limit) &&
>>         a * b <= limit * limit)
>>
>> Since these are all non-negative integers, this can be simplified to
>>
>>     if (a * b <= limit * limit)
>
> Makes sense.

I've always assumed that the original was for correctness (if a and b are both larger than limit, a*b could end up being smaller than limit*limit when the result of multiplication of the former wraps around but not the latter) ...

Show 9 quoted lines

>> The only advantage of the former would be in avoiding a couple
>> multiplications in the rare case that both a and b are BOTH very large.
>> I see no reason for such an optimization given that this code is not in
>> any kind of loop.  Prefer code simplicity here and change to the latter
>> form.
>
> If you were really paranoid, you could perform these checks with
> unsigned_mult_overflows(), but I don't think that it's worth doing so
> here.

... and in no way as an optimization.

So, I dunno.