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 >