On Wed, Dec 9, 2020 at 6:03 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Taylor Blau <me@xxxxxxxxxxxx> writes: > > > On Sun, Dec 06, 2020 at 02:54:31AM +0000, Elijah Newren via GitGitGadget wrote: > >> From: Elijah Newren <newren@xxxxxxxxx> > >> > >> diffcore-rename had two different checks of the form > >> > >> if ((a < limit || b < limit) && > >> a * b <= limit * limit) > >> > >> Since these are all non-negative integers, this can be simplified to > >> > >> if (a * b <= limit * limit) > > > > Makes sense. > > I've always assumed that the original was for correctness (if a and > b are both larger than limit, a*b could end up being smaller than > limit*limit when the result of multiplication of the former wraps > around but not the latter) ... > > >> The only advantage of the former would be in avoiding a couple > >> multiplications in the rare case that both a and b are BOTH very large. > >> I see no reason for such an optimization given that this code is not in > >> any kind of loop. Prefer code simplicity here and change to the latter > >> form. > > > > If you were really paranoid, you could perform these checks with > > unsigned_mult_overflows(), but I don't think that it's worth doing so > > here. > > ... and in no way as an optimization. > > So, I dunno. Ah, so would you be okay replacing these with if (st_mult(num_targets, limited_sources) <= st_mult(rename_limit, rename_limit)) ? That'd make the correctness intent far clearer, and allow us to drop the casting as well since st_mult converts to size_t. (If, on the off chance you're on a platform where size_t is 32-bits and the multiplications don't fit in that size, then the requested matrices for computing rename detection won't fit in memory for you.)
