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. >

## Advertising

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 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. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers