Re: Add numeric_trim(numeric)

On 27 December 2015 at 07:11, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> 2015-11-19 3:58 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>>> Here's a patch for the second function suggested in
>>> 5643125E.1030605@joh.to.
>>>
> So I am sending a review of this patch.
>

I took a quick look at this too.

It seems like a useful function to have, but perhaps it should just be
called trim() rather than numeric_trim(), for consistency with the
names of the other numeric functions, which don't start with
"numeric_".


I found a bug in the dscale calculation:

select numeric_trim(1e100);
ERROR:  value overflows numeric format

The problem is that the trailing zeros are already stripped off, and
so the computation
   dscale = (arg.ndigits - arg.weight - 1) * DEC_DIGITS;

results in a negative dscale, which needs to be guarded against.


Actually I found the implementation a little confusing, partly because
the first comment doesn't really match the line of code that follows
it, and partly because of the complexity of the loop, the macros and
working out all the exit conditions from the loop. A much simpler
implementation would be to first call strip_var(), and then you'd only
need to inspect the final digit (known to be non-zero). In
pseudo-code, it might look a little like the following:
   init_var_from_num()   strip_var()   if ndigits > 0:       dscale = (arg.ndigits - arg.weight - 1) * DEC_DIGITS
ifdscale < 0:           dscale = 0       if dscale > 0:           // Here we know the last digit is non-zero and that
itis to the           // right of the decimal point, so inspect it and adjust dscale           // (reducing it by up to
3)to trim any remaining zero decimal           // digits           dscale -= ...   else:       dscale = 0
 

This then only has to test for non-zero decimal digits in the final
base-NBASE digit, rather than looping over digits from the right. In
addition, it removes the need to do the following at the start:
   /* for simplicity in the loop below, do full NBASE digits at a time */   dscale = ((arg.dscale + DEC_DIGITS - 1) /
DEC_DIGITS)* DEC_DIGITS;
 

which is the comment I found confusing because doesn't really match up
with what that line of code is doing.

Also, it then won't be necessary to do
   /* If it's zero, normalize the sign and weight */   if (ndigits == 0)   {       arg.sign = NUMERIC_POS;
arg.weight= 0;       arg.dscale = 0;   }
 

because strip_var() will have taken care of that.

In fact, in most (all?) cases, the equivalent of strip_var() will have
already been called by whatever produced the input Numeric, so it
won't actually have any work to do, but it will fall through very
quickly in those cases.


One final comment -- in the regression tests the quotes around all the
numbers are unnecessary.

Regards,
Dean