On 14/08/2024 06:07, Nathan Bossart wrote:
> On Tue, Aug 13, 2024 at 04:46:34PM -0500, Nathan Bossart wrote:
>> I've been preparing 0001 for commit. I've attached what I have so far.
>>
>> The main changes are the implementations of pg_abs_* and pg_neg_*. For the
>> former, I've used abs()/i64abs() for the short/int implementations. For
>> the latter, I've tried to use __builtin_sub_overflow() when possible, as
>> that appears to produce slightly better code. When
>> __builtin_sub_overflow() is not available, the values are upcasted before
>> negation, and we check that result before casting to the return type. That
>> approach more closely matches the surrounding functions. (One exception is
>> pg_neg_u64_overflow() when we have neither HAVE__BUILTIN_OP_OVERFLOW nor
>> HAVE_INT128. In that case, we have to hand-roll everything.)
>
> 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:
> * - 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.
> +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.
There's also some code in libpq's pqCheckOutBufferSpace() function that
could use these functions.
--
Heikki Linnakangas
Neon (https://neon.tech)
