Re: [PATCH 4/4] config: avoid calling `labs()` on too-large data type

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2019, 22:26 UTC
Message-ID: <xmqq7e9ltbe2.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <90ef3797-78a6-f6d9-443b-387c0ab7cbe7@web.de>

René Scharfe <l.s.r@web.de> writes:

>>>> To fix it, let's just "unroll" the function (i.e. negate the value if it
>>>> is negative).
>>>
>>> There's also imaxabs(3).

That may be true, but seeing that some platforms wants to see intmax_t defined in the compat/ layer, I suspect we cannot avoid having a copy of unrolled implementation somewhere in our code.

Show 15 quoted lines

>>>> +		uval = val < 0 ? -val : val;
>>>>  		uval *= factor;
>>>> -		if (uval > max || labs(val) > uval) {
>>>> +		if (uval > max || (val < 0 ? -val : val) > uval) {
>>>>  			errno = ERANGE;
>>>>  			return 0;
>>>>  		}
>>>
>>> So this check uses unsigned arithmetic to find out if the multiplication
>>> overflows, right?...
>> No, this example is wrong...
> ...
> Anyway, the code would be easier to read and ready for any units if it
> used unsigned_mult_overflows; would have saved me time spent painfully
> wading through the math at least.

A patch to use unsigned_mult_overflows() here, on top of the "unrolled imaxabs" patch we reviewed, would be good to tie a loose end.

Thanks.