Re: Remove dependence on integer wrapping

On Wed, Aug 14, 2024 at 10:02:28AM +0300, Heikki Linnakangas wrote:
> On 14/08/2024 06:07, Nathan Bossart wrote:
>> And here's a new version of the patch in which I've attempted to fix the
>> silly mistakes.
>
> LGTM, just a few small comments:

Thanks for reviewing.

>> * - 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.

>> +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.

> There's also some code in libpq's pqCheckOutBufferSpace() function that
> could use these functions.

Duly noted.

--
nathan