On Wed, Aug 7, 2024 at 11:08 AM Nathan Bossart <
nathandbossart@gmail.com> wrote:
>
> I started looking at 0001 again with the intent of committing it, and this
> caught my eye:
>
> - /* make the amount positive for digit-reconstruction loop */
> - value = -value;
> + /*
> + * make the amount positive for digit-reconstruction loop, we can
> + * leave INT64_MIN unchanged
> + */
> + pg_neg_s64_overflow(value, &value);
>
> The comment mentions that we can leave the minimum value unchanged, but it
> doesn't explain why. Can we explain why?
I went back to try and figure this out and realized that it would be
much simpler to just convert value to an unsigned integer and not worry
about overflow. So I've updated the patch to do that.
> +static inline bool
> +pg_neg_s64_overflow(int64 a, int64 *result)
> +{
> + if (unlikely(a == PG_INT64_MIN))
> + {
> + return true;
> + }
> + else
> + {
> + *result = -a;
> + return false;
> + }
> +}
>
> Can we add a comment that these routines do not set "result" when true is
> returned?
I've added a comment to the top of the file where we describe the
return values of the other functions.
I also updated the implementations of the pg_abs_sX() functions to
something a bit simpler. This was based on feedback in another patch
[0], and more closely matches similar logic in other places.
Thanks,
Joseph Koshakow
[0]
https://postgr.es/m/CAAvxfHdTsMZPWEHUrZ=h3cky9Ccc3Mtx2whUHygY+ABP-mCmUw@mail.gmail.co
