Hi
2016-01-06 16:21 GMT+01:00 Dean Rasheed <dean.a.rash...@gmail.com>:
> On 27 December 2015 at 07:11, Pavel Stehule <pavel.steh...@gmail.com>
> wrote:
> >> 2015-11-19 3:58 GMT+01:00 Marko Tiikkaja <ma...@joh.to>:
> >>> Here's a patch for the second function suggested in
> >>> 5643125e.1030...@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
>
my oversight :(
>
> 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
> if dscale < 0:
> dscale = 0
> if dscale > 0:
> // Here we know the last digit is non-zero and that it is 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.
>
so I changed status of this patch to "waiting on author"
Thank you
Pavel
>
> Regards,
> Dean
>
