Zhihong Yu <z...@yugabyte.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
