Re: avoid negating LONG_MIN in cash_out()

On Thu, Aug 11, 2022 at 6:28 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Zhihong Yu <zyu(at)yugabyte(dot)com> writes:
> > How about patch v2 which uses the same check from cash_in() ?
>
> I'm not sure which part of this statement you're not getting:
> it is completely unacceptable for cash_out to fail on valid
> values of the type. And this value is valid. cash_in goes
> out of its way to take it, and you can also produce it via
> arithmetic operators.
>
> I understand that you're trying to get rid of an analyzer warning that
> negating INT64_MIN is (pedantically, not in practice) undefined behavior.
> But the way to fix that is to make the behavior conform to the C spec.
> Perhaps it would work to do
>
> Cash value = PG_GETARG_CASH(0);
> uint64 uvalue;
>
> if (value < 0)
> uvalue = -(uint64) value;
> else
> uvalue = value;
>
> and then use uvalue instead of "(uint64) value" in the loop.
> Of course, this begs the question of what negation means for
> an unsigned value. I believe that this formulation is allowed
> by the C spec and produces the same results as what we do now,
> but I'm not convinced that it's clearer for the reader.
>
> Another possibility is
>
> if (value < 0)
> {
> if (value == INT64_MIN)
> uvalue = however you wanna spell -INT64_MIN;
> else
> uvalue = (uint64) -value;
> }
> else
> uvalue = value;
>
> but this really seems to be letting pedantry get the best of us.
>
> The short answer here is that the code works fine on every platform
> we support. We know that because we have a regression test checking
> this exact case. So it's not broken and I don't think there's a
> very good argument that it needs to be fixed. Maybe the right thing
> is just to add a comment pointing out what happens for INT64_MIN.
>
> regards, tom lane
>
Hi,
Thanks for taking the time to contemplate various possibilities.

I thought of using uint64 as well - but as you have shown, the readability
isn't better.

I will keep this in the back of my mind.

Cheers