Re: patch ping Re: [java] Fix BigDecimal ROUND_HALF_EVEN

[Per Bothner <per at bothner dot com>]

Mark Wielaard wrote:

> You need half also to see if ROUND_UNNECESSARY can be satisfied.

I don't understand this.

> So the code becomes a bit messy with first two if statements for
> ROUND_CEILING and ROUND_FLOOR and then a big else block with the half
> calculation and a big switch, but I think it is still clear what the
> code does.

Ok.

> Another optimization can be done by noticing that with the new code we
> can just look at the remainder and be done when it is zero. That also
> makes that the ROUND_UNNECESSARY case can be pulled out of the big
> switch before even the sign is calculated.

Yes, that was my intent.

> > This (in the original code) looks questionable:
> >     int sign = unrounded.signum ();
> > Consider the test in ROUND_CEILING:
> > 	roundingMode = (sign == 1) ? ROUND_UP : ROUND_DOWN;
> > Shouldn't this be whether the this.signum() is positive?
> > Consider dividing -9 by 10.  unrounded.signum () is 0,
> > so we round down when we should be rounding up.
>
> In the above case unrounded.signum () would be -1.
> I believe the original is correct.

Ok, consider -9 divided by 100 instead (with all scales zero).

> > Have you tested it?
>
> There already were some 600 BigDecimal tests for Mauve. I added a couple
> more for some ROUND_HALF_EVEN corner cases (positive/negative, odd/even,
> different nearest neigbors or equidistant). All these tests pass with
> the patch below.

Do you test negative values of val as well?

> OK to commit?

Yes, but may I suggest some minor optimizations?

Instead of sign == -1 or sign == 1 it is probably more efficient to do 
sign < 0 or sign > 0.

>      if (roundingMode == ROUND_UP)
> -      return new BigDecimal (unrounded.add (BigInteger.valueOf (1)), newScale);
> +      return new BigDecimal (unrounded.add (BigInteger.valueOf
> +					    (sign != 0 ? sign : 1)), newScale);
>  
>      // roundingMode == ROUND_DOWN
>      return new BigDecimal (unrounded, newScale);


    if (roundingMode == ROUND_UP)
      {
        unrounded = unrounded.add(sign >= 0 ? BigInteger.ONE : 
BigInteger.valueOf(-1));
      }
    return new BigDecimal (unrounded, newScale);

Calling new BigDecimal only once reduce code space.
The change to the operand to add is more arguable.
--
	--Per Bothner
per@bothner.com   http://per.bothner.com/