Re: [PATCH v4 4/6] xdiff/xdl_cleanup_records: make limits more clear
On 14/04/2026 23:15, Junio C Hamano wrote:
Show 18 quoted lines
> 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
Show 28 quoted lines
>
> #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.
>