From: Nathan Bossart <nathandbossart@gmail.com>
To: Joseph Koshakow <koshy44@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Remove dependence on integer wrapping
Date: Tue, 11 Jun 2024 11:22:27 -0500
Message-ID: <Zmh5wxds3DlyWAZy@nathan> (raw)
In-Reply-To: <CAAvxfHdsdqgSoAbG-yMW186PcaicNQ2Dcr=7W8Misoy_y53fsg@mail.gmail.com>
References: <CAAvxfHdBPOyEGS7s+xf4iaW0-cgiq25jpYdWBqQqvLtLe_t6tw@mail.gmail.com>
<ZmZbXK1CNMbkty0W@nathan>
<618777.1717984674@sss.pgh.pa.us>
<20240610183031.fxyh5lug2i273jj4@awork3.anarazel.de>
<CAAvxfHdsdqgSoAbG-yMW186PcaicNQ2Dcr=7W8Misoy_y53fsg@mail.gmail.com>
On Tue, Jun 11, 2024 at 09:31:39AM -0400, Joseph Koshakow wrote:
> tmp is an uint16 here, it seems like you might have read it as an
> int16? We would need some helper method like
>
> static inline bool
> pg_neg_u16_overflow(uint16 a, int16 *result);
>
> and then we could replace that whole chunk with something like
>
> if (unlikely(pg_neg_u16_overflow(tmp, &result)))
> goto out_of_range;
> else
> return result;
>
>
> that pattern shows up a lot in this file, but I was worried that it
> wasn't useful as a general purpose function. Happy to add it
> though if you still feel otherwise.
I personally find that much easier to read. Since the existing open-coded
overflow check is apparently insufficient, I think there's a reasonably
strong case for centralizing this sort of thing so that we don't continue
to make the same mistakes.
>> Ah, I see. Joe's patch does that in one place. It's probably worth doing
>> that in the other places this "just-barefly overflow" comment appears
>> IMHO.
>
> The only other place I found this comment was in
> `make_timestamp_internal`. I've updated that function and added some
> tests. I also manually verified that the behavior matches before and
> after this patch.
tm2timestamp() in src/interfaces/ecpg/pgtypeslib/timestamp.c has the same
comment. The code there looks very similar to the code for tm2timestamp()
in the other timestamp.c...
--
nathan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, koshy44@gmail.com, andres@anarazel.de, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: Remove dependence on integer wrapping
In-Reply-To: <Zmh5wxds3DlyWAZy@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox