Re: Remove dependence on integer wrapping

On 14/08/2024 20:20, Nathan Bossart wrote:
> On Wed, Aug 14, 2024 at 10:02:28AM +0300, Heikki Linnakangas wrote:
>> On 14/08/2024 06:07, Nathan Bossart wrote:
>>> * - If a * b overflows, return true, otherwise store the result of a * b
>>> * into *result. The content of *result is implementation defined in case of
>>> * overflow.
>>> + * - If -a overflows, return true, otherwise store the result of -a into
>>> + * *result. The content of *result is implementation defined in case of
>>> + * overflow.
>>> + * - Return the absolute value of a as an unsigned integer of the same
>>> + * width.
>>> *---------
>>> */
>>
>> The last "Return the absolute value of a ..." sentence feels a bit weird. In
>> all the preceding sentences, 'a' is part of an "If a" sentence that defines
>> what 'a' is. In the last one, it's kind of just hanging there.
>
> How about:
>
> If a is negative, return -a, otherwise return a. Overflow cannot occur
> because the return value is an unsigned integer with the same width as
> the argument.

Hmm, that still doesn't say what operation it's referring to. They
existing comments say "a + b", "a - b" or "a * b", but this one isn't
referring to anything at all. IMHO the existing comments are not too
clear on that either though. How about something like this:

/*---------
*
* The following guidelines apply to all the *_overflow routines:
*
* If the result overflows, return true, otherwise store the result into
* *result. The content of *result is implementation defined in case of
* overflow
*
* bool pg_add_*_overflow(a, b, *result)
*
* Calculate a + b
*
* bool pg_sub_*_overflow(a, b, *result)
*
* Calculate a - b
*
* bool pg_mul_*_overflow(a, b, *result)
*
* Calculate a * b
*
* bool pg_neg_*_overflow(a, *result)
*
* Calculate -a
*
*
* In addition, this file contains:
*
* <unsigned int type> pg_abs_*(<signed int type> a)
*
* Calculate absolute value of a. Unlike the standard library abs() and
* labs() functions, the the return type is unsigned, and the operation
* cannot overflow.
*---------
*/

>>> +static inline uint16
>>> +pg_abs_s16(int16 a)
>>> +{
>>> + return abs(a);
>>> +}
>>> +
>>
>> This is correct, but it took me a while to understand why. Maybe some
>> comments would be in order.
>>
>> The function it calls is "int abs(int)". So this first widens the int16 to
>> int32, and then narrows the result from int32 to uint16.
>>
>> The man page for abs() says "Trying to take the absolute value of the most
>> negative integer is not defined." That's OK in this case, because that
>> refers to the most negative int32 value, and the argument here is int16. But
>> that's why the pg_abs_s64(int64) function needs the special check for the
>> most negative value.
>
> Yeah, I've added some casts/comments to make this clear. I got too excited
> about trimming it down and ended up obfuscating these important details.

That's better, thanks!

--
Heikki Linnakangas
Neon (https://neon.tech)