Re: Remove dependence on integer wrapping

On Tue, Jul 16, 2024 at 1:57 PM Nathan Bossart <nathandbossart@gmail.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@gmail.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.


Thanks,
Joe Koshakow