From: Phillip Wood Date: Wed, 15 Apr 2026 13:54:40 GMT Subject: Re: [PATCH v4 4/6] xdiff/xdl_cleanup_records: make limits more clear Message-ID: <50ea6e41-6b29-46ad-aa97-0eaa289db7cf@gmail.com> In-Reply-To: On 14/04/2026 23:15, Junio C Hamano wrote: > Ezekiel Newren 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. >