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)
