Re: [PATCH 2/4] Remove silent clamp of renameLimit

From: Elijah Newren <newren@gmail.com>
Date: Nov 10, 2017, 18:36 UTC
Message-ID: <CABPp-BGwoduH_T7U4uN5A-h43wREfXdQgV8YCVWf8w5N09cYSw@mail.gmail.com>
In-Reply-To: <CAGZ79kbqxA9U7EsU6_i895vgrTUwmqv8f02NCoUqPS1_nD2PtA@mail.gmail.com>

Thanks for taking a look!

On Fri, Nov 10, 2017 at 10:26 AM, Stefan Beller <sbeller@google.com> wrote: <snip>

Show 12 quoted lines

>> -       if (rename_limit <= 0 || rename_limit > 32767)
>> -               rename_limit = 32767;
>>         if ((num_create <= rename_limit || num_src <= rename_limit) &&
>> -           (num_create * num_src <= rename_limit * rename_limit))
>> +           ((double)num_create * (double)num_src
>> +            <= (double)rename_limit * (double)rename_limit))
>>                 return 0;
>
> From a technical perspective, I would think that if
> (num_create <= rename_limit || num_src <= rename_limit)
> holds true, that the double-cast condition would also be always true?
> Could we just remove that last check?

Not necessarily. For example, if num_create = rename_limit-1 and num_src = rename_limit+2, then the first condition will be satisfied but the second won't. If it was && rather than ||, then the second condition would be superfluous.

> Or phrased differently, if we can cast to double and extend the check
> here, do we have to adapt code at other places as well?

Good point, and yes. Perhaps I should have re-ordered my patch series because I came back to it later and realized that the progress code was broken due to overflow/wraparound, and a patch 3 fixed that.

Further, the later patch used uint64_t instead of double. While double works, perhaps I should change the double here to uint64_t for consistency?