Re: Remove dependence on integer wrapping

On Tue, Jul 16, 2024 at 1:57 PM Nathan Bossart <nathandbossart(at)gmail(dot)com>
wrote:
>>
>> On Mon, Jul 15, 2024 at 07:55:22PM -0400, Joseph Koshakow wrote:
>> On Mon, Jul 15, 2024 at 11:31 AM Nathan Bossart <nathandbossart(at)gmail(dot)com
>
>> wrote:
>>> I'm curious why you aren't using float8_mul/float8_div here, i.e.,
>>>
>>> fresult = rint(float8_mul((float8) c, f));
>>> fresult = rint(float8_div((float8) c, f));
>>
>> I wrongly assumed that it was only meant to be used to implement
>> multiplication and division for the built-in float types. I've updated
>> the patch to use these functions.
>
> The reason I suggested this is so that we could omit all the prerequisite
> isinf(), isnan(), etc. checks in the cash_mul_float8() and friends. The
> checks are slighly different, but from a quick glance it just looks like
we
> might end up relying on the FLOAT8_FITS_IN_INT64 check in more cases.

I don't think we can omit the prerequisite isnan() checks. Neither
float8_mul() nor float8_div() reject nan inputs/result, and
FLOAT8_FITS_IN_INT64 has the following to say about inf and nan

These macros will do the right thing for Inf, but not necessarily for
NaN,
so check isnan(num) first if that's a possibility.

Though, I think you're right that we can remove the isinf() check from
cash_mul_float8(). That check is fully covered by FLOAT8_FITS_IN_INT64,
since all infinite inputs will result in an infinite output. That also
makes the infinite result check in float8_mul() redundant.
Additionally, I believe that the underflow check in float8_mul() is
unnecessary. val1 is an int64 casted to a float8, so it can never be
-1 < val < 1, so it can never cause an underflow to 0. So I went ahead
and removed float8_mul() since all of its checks are redundant.

For cash_div_float8() we have a choice. The isinf() input check
protects against the following, which is not rejected by any of
the other checks.

test=# SELECT '5'::money / 'inf'::float8;
?column?
----------
$0.00
(1 row)

For now, I've kept the isinf() input check to reject the above query,
let me know if you think we should allow this.

The infinite check in float8_div() is redundant because it's covered
by FLOAT8_FITS_IN_INT64. Also, the underflow check in float8_div() is
unnecessary for similar reasons to float8_mul(). So if we continue to
have a divide by zero check in cash_div_float8(), then we can remove
float8_div() as well.

>>> and "cash_sub_int64"
>>
>> Did you mean "cash_div_int64"? There's only a single function that
>> subtracts cash and an integer, but there's multiple functions that
>> divide cash by an integer. I've added a "cash_div_int64" in the updated
>> patch.
>
> My personal preference would be to add helper functions for each of these
> so that all the overflow, etc. checks are centralized in one place and
> don't clutter the calling code. Plus, it might help ensure error
> handling/messages remain consistent.

Ah, OK. I've added helpers for both subtraction and addition then.

> +static Cash
> +cash_mul_float8(Cash c, float8 f)
>
> nitpick: Can you mark these "inline"? I imagine most compilers inline
them
> without any prompting, but we might as well make our intent clear.

Updated in the attached patch.

Once again, the other patches, 0001, 0003, and 0004 are unchanged but
have their version number incremented.

Thanks,
Joe Koshakow