From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-committers(at)postgresql(dot)org |
Subject: | Re: pgsql: Use new overflow aware integer operations. |
Date: | 2017-12-29 20:21:54 |
Message-ID: | 20171229202154.crqrq3s7ssqyqn5s@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-committers |
On 2017-12-27 17:59:26 -0500, Tom Lane wrote:
> [ back from Christmas break ]
>
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On December 22, 2017 7:52:54 PM GMT+01:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> I will not accept an implementation that spews compiler warnings
> >> all over the place, which is what this one is doing. Please fix that,
> >> or else I will.
>
> > Are you seriously implying that I'm suggesting that we live with a warning / that I refuse to fix one? All I was saying is that I don't want to exactly define which value *result is set to in case of overflow. Without having resolved the discussion of semantics it just seemed pointless to start fixing...
>
> Sorry, I was being unnecessarily grumpy there.
Lack of holidays does that to one ;)
> I follow your point about not wanting to constrain the implementation
> to yield the correct low-order bits if we don't actually need that
> behavior ... but I'm still not happy about the warnings.
Agreed.
> What do you think of having the fallback code explicitly set the output
> variable to zero (or any other fixed value) on overflow, like
>
> #if defined(HAVE__BUILTIN_OP_OVERFLOW)
> return __builtin_add_overflow(a, b, result);
> #else
> int32 res = (int32) a + (int32) b;
>
> if (res > PG_INT16_MAX || res < PG_INT16_MIN)
> + {
> + *result = 0; /* just to keep compiler quiet */
> return true;
> + }
> *result = (int16) res;
> return false;
> #endif
>
> I do not think this would cause any performance loss in our expected
> usage, because reasonably bright compilers would detect that the store
> is dead code and remove it. But less-bright compilers would not be
> issuing warnings.
Yea, that works for me. I wonder if we should choose an absurd sentinel
value to prevent code from relying on one? 0x0000beef or such. Unless
somebody protests soon-ish I'll make it so.
Greetings,
Andres Freund
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2017-12-29 20:48:19 | pgsql: Rely on executor utils to build targetlist for DML RETURNING. |
Previous Message | Andres Freund | 2017-12-29 19:43:23 | Re: pgsql: Use new overflow aware integer operations. |