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

[Taylor Blau <me@ttaylorr.com>]

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).

>  		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.

Thanks,
Taylor