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

From: Elijah Newren <newren@gmail.com>
Date: Dec 10, 2020, 02:17 UTC
Message-ID: <CABPp-BFcSV_LPy-_AW+c-1OgzAyHfEM7D+YL+fFwxYKDheqxEg@mail.gmail.com>
In-Reply-To: <xmqqtusuxvr8.fsf@gitster.c.googlers.com>

On Wed, Dec 9, 2020 at 6:03 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 35 quoted lines

>
> Taylor Blau <me@ttaylorr.com> writes:
>
> > 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) ...
>
> >> 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.

Ah, so would you be okay replacing these with

   if (st_mult(num_targets, limited_sources) <= st_mult(rename_limit,
rename_limit))

That'd make the correctness intent far clearer, and allow us to drop the casting as well since st_mult converts to size_t. (If, on the off chance you're on a platform where size_t is 32-bits and the multiplications don't fit in that size, then the requested matrices for computing rename detection won't fit in memory for you.)