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

[Elijah Newren <newren@gmail.com>]

On Wed, Dec 9, 2020 at 2:10 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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.
>
> > 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.
>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  diffcore-rename.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/diffcore-rename.c b/diffcore-rename.c
> > index 68ddf51a2a1..0f8fce9293e 100644
> > --- a/diffcore-rename.c
> > +++ b/diffcore-rename.c
> > @@ -450,9 +450,8 @@ static int too_many_rename_candidates(int num_targets, int num_sources,
> >        */
> >       if (rename_limit <= 0)
> >               rename_limit = 32767;
> > -     if ((num_targets <= rename_limit || num_sources <= rename_limit) &&
> > -         ((uint64_t)num_targets * (uint64_t)num_sources
> > -          <= (uint64_t)rename_limit * (uint64_t)rename_limit))
> > +     if ((uint64_t)num_targets * (uint64_t)num_sources
> > +         <= (uint64_t)rename_limit * (uint64_t)rename_limit)
>
> One small nit here (and below) is that not all of these need casting.
> Only one operand of each multiplication needs to be widened for the
> compiler to widen the other, too. So, I'd write this instead as:
>
> > +     if ((num_targets * (uint64_t)num_sources) <=
> > +     (rename_limit * (uint64_t)rename_limit))
>
> or something. (I tend to prefer the cast on the right-most operand,
> since it makes clear that there's no need to cast the "first" operand,
> and that casting either will do the trick).

Yeah, I think that reads slightly better too, but on the other hand it
does make the diff slightly harder to read.  *shrug*.

> >               return 0;
> >
> >       options->needed_rename_limit =
> > @@ -468,9 +467,8 @@ static int too_many_rename_candidates(int num_targets, int num_sources,
> >                       continue;
> >               limited_sources++;
> >       }
> > -     if ((num_targets <= rename_limit || limited_sources <= rename_limit) &&
> > -         ((uint64_t)num_targets * (uint64_t)limited_sources
> > -          <= (uint64_t)rename_limit * (uint64_t)rename_limit))
> > +     if ((uint64_t)num_targets * (uint64_t)limited_sources
> > +         <= (uint64_t)rename_limit * (uint64_t)rename_limit)
>
> Same notes here, of course. I was hoping that we could only do this
> multiplication once, but it looks like limited_sources grows between the
> two checks, so we have to repeat it here, I suppose.

The right hand side of the expression is the same -- rename_limit *
rename_limit -- so it could be stored off (though I don't think
there's much point in doing so unless it made the code clearer; this
is not remotely close to a hot path).  However, in the left hand side,
it's not so much that one of the variables has changed since the last
check, it's that it uses a different set of variables in the check.
In particular, it replaces 'num_sources' with 'limited_sources'.