Re: Remove dependence on integer wrapping - Mailing list pgsql-hackers

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)