On Mon, Jul 15, 2024 at 07:55:22PM -0400, Joseph Koshakow wrote: > On Mon, Jul 15, 2024 at 11:31 AM Nathan Bossart <nathandboss...@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. >> 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. +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. -- nathan
