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

From: Taylor Blau <me@ttaylorr.com>
Date: Dec 9, 2020, 22:10 UTC
Message-ID: <X9FLaiuWpYely6es@nand.local>
In-Reply-To: <f96bb36930a6e5e42f0d3b9c5dfa3b2cc27c1f9d.1607223276.git.gitgitgadget@gmail.com>

On Sun, Dec 06, 2020 at 02:54:31AM +0000, Elijah Newren via GitGitGadget wrote:

Show 10 quoted lines

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

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

Show 18 quoted lines

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

Show 12 quoted lines

>  		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