Date: Tue, 16 Jul 2024 12:57:47 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Joseph Koshakow <koshy44@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Andres Freund <andres@anarazel.de>
Subject: Re: Remove dependence on integer wrapping
Message-ID: <Zpa0m_OVw_iWYicV@nathan>
References: <31ad2cd1-db94-bdb3-f91a-65ffdb4bef95@gmail.com>
 <CAAvxfHfqJca-WGvWJQ3ZAtt2po=mkQzU8r9Qjzom4YqxQi84oA@mail.gmail.com>
 <CAAvxfHdhEz4eF7z1wNn_4C4Gkt+e-h7OjzKUJXt7CE+CvubqcQ@mail.gmail.com>
 <CAAvxfHdze21+UZPWo7ePfH19-9hvXe_9SEZk4HABPHuzaUitRQ@mail.gmail.com>
 <CAAvxfHd22ujbY6uTraE+ZSTUN0xX0G-=NZcr0YnKSQuXthcM+g@mail.gmail.com>
 <CAAvxfHcDQEziyQ-ehv0Bg05OovGPGO95VoO7-DyxJx6Tkq85vA@mail.gmail.com>
 <ZpFeopUrhdB5GgNN@nathan>
 <CAAvxfHdX-t03LvxROxbyodBrtzBzZQEKdKB0FCpEr+p1JnFxsA@mail.gmail.com>
 <ZpVAvLbtUPnK1JKe@nathan>
 <CAAvxfHdYnTB7BjkRSPO8__wxB3880TiYD-TVU2uW8q1KbzymPg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAAvxfHdYnTB7BjkRSPO8__wxB3880TiYD-TVU2uW8q1KbzymPg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Zpa0m_OVw_iWYicV%40nathan>
Precedence: bulk

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.

>>    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