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

[Junio C Hamano <gitster@pobox.com>]

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.

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