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)
