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>
> >> 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
> 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:
> 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
> // 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 -= ...
> 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"