Re: [PATCH v4 4/6] xdiff/xdl_cleanup_records: make limits more clear

[Phillip Wood <phillip.wood123@gmail.com>]

On 14/04/2026 23:15, Junio C Hamano wrote:
> Ezekiel Newren <ezekielnewren@gmail.com> writes:
> 
>>>> +             mlim1 = XDL_MIN(xdl_bogosqrt(xdf1->nrec), XDL_MAX_EQLIMIT);
>>>> +             mlim2 = XDL_MIN(xdl_bogosqrt(xdf2->nrec), XDL_MAX_EQLIMIT);
>>>
>>> As Junio has pointed out we now evaluate xdl_bogosqrt() twice which is
>>> unfortunate. It would have been nice to mention that in the commit
>>> message and explain why it does not matter.
>>
>> It doesn't matter because xdl_bogosqrt() was being called twice before
>> and is being called twice now. There is no change in that regard.
>> That's why I split mlim into 2 variables to make it more clear.
>>
>> It looks like you and Junio have both missed that xdl_bogo_sqrt() is
>> being called on different values.
> 
> I think Phillip's point is that XDL_MIN(a, b) would evaluate (a)
> twice.

Exactly, thanks for clarifying

Phillip

> 
> 	#define XDL_MIN(a, b) ((a) < (b) ? (a): (b))
> 
> So the code you have above
> 
> 	mlim1 = XDL_MIN(xdl_bogosqrt(xdf1->nrec), XDL_MAX_EQLIMIT);
> 
> actually is
> 
> 	mlim1 = ((xdl_bogosqrt(xdf1->nrec) < XDL_MAX_EQLIMIT)
> 		? xdl_bogosqrt(xdf1->nrec)
> 		: XDL_MAX_EQLIMIT);
> 
> If you are lucky and xdf1->nrec is so large, there is only one call
> to xdl_bogosqrt() before mlim1 gets assigned XDL_MAX_EQLIMIT, but
> usually you'll call it on the same xdf1->nrec twice before you
> assign the result to mlim1, no?
> 
> The original lost by the patch looked like this:
> 
>   	/*
>   	 * Initialize temporary arrays with DISCARD, KEEP, or INVESTIGATE.
>   	 */
> -	if ((mlim = (long)xdl_bogosqrt((uint64_t)xdf1->nrec)) > XDL_MAX_EQLIMIT)
> -		mlim = XDL_MAX_EQLIMIT;
> 
> which computed it once, assigned it to mlim, and then clamped.
>