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

[Mark Wielaard <mark at klomp dot org>]

Hi,

On Sun, 2003-07-20 at 00:23, Per Bothner wrote:
> Mark Wielaard wrote:
> 
> > You need half also to see if ROUND_UNNECESSARY can be satisfied.
> 
> I don't understand this.

Sorry, ignore me. That was with the old code. The new version has
ROUND_UNNECESSARY optimized so that it doesn't use half at all.

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

Aha. I see what you are getting at. Indeed unrounded.signum() is the
wrong sign. But this.signum() is also not correct in for example
9 divided by -100. So (since the zero cases are already taken care of)
sign should actually be intVal.signum () * valIntVal.signum ().
Fixed and an explicit test added to Mauve.

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

I have now added tests for all forms of divide (pos/neg, different
rounding modes and scales) to Mauve. There was one additional thing to
fix in the calcultation of half (both values should be positive before
comparing). Also fixed. All 164 new cases now succeed.

> > OK to commit?
> 
> Yes, but may I suggest some minor optimizations?

All added. Final patch as committed attached.

Cheers,

Mark

Index: java/math/BigDecimal.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/math/BigDecimal.java,v
retrieving revision 1.9
diff -u -r1.9 BigDecimal.java
--- java/math/BigDecimal.java	19 Apr 2003 19:26:41 -0000	1.9
+++ java/math/BigDecimal.java	1 Aug 2003 12:56:31 -0000
@@ -1,5 +1,5 @@
 /* java.math.BigDecimal -- Arbitrary precision decimals.
-   Copyright (C) 1999, 2000, 2001 Free Software Foundation, Inc.
+   Copyright (C) 1999, 2000, 2001, 2003 Free Software Foundation, Inc.
 
 This file is part of GNU Classpath.
 
@@ -273,7 +273,7 @@
     // Ensure that pow gets a non-negative value.
     int valScale = val.scale;
     BigInteger valIntVal = val.intVal;
-    int power = newScale + 1 - (scale - val.scale);
+    int power = newScale - (scale - val.scale);
     if (power < 0)
       {
 	// Effectively increase the scale of val to avoid an
@@ -285,50 +285,53 @@
     BigInteger dividend = intVal.multiply (BigInteger.valueOf (10).pow (power));
     
     BigInteger parts[] = dividend.divideAndRemainder (valIntVal);
-//      System.out.println("int: " + parts[0]);
-//      System.out.println("rem: " + parts[1]);
 
-    int roundDigit = parts[0].mod (BigInteger.valueOf (10)).intValue ();
-    BigInteger unrounded = parts[0].divide (BigInteger.valueOf (10));
-
-    if (roundDigit == 0 && parts[1].signum () == 0) // no rounding necessary
+    BigInteger unrounded = parts[0];
+    if (parts[1].signum () == 0) // no remainder, no rounding necessary
       return new BigDecimal (unrounded, newScale);
 
-    int sign = unrounded.signum ();
+    if (roundingMode == ROUND_UNNECESSARY)
+      throw new ArithmeticException ("newScale is not large enough");
+
+    int sign = intVal.signum () * valIntVal.signum ();
 
-    switch (roundingMode)
+    if (roundingMode == ROUND_CEILING)
+      roundingMode = (sign > 0) ? ROUND_UP : ROUND_DOWN;
+    else if (roundingMode == ROUND_FLOOR)
+      roundingMode = (sign < 0) ? ROUND_UP : ROUND_DOWN;
+    else
       {
-      case ROUND_UNNECESSARY:
-	throw new ArithmeticException ("newScale is not large enough");
-      case ROUND_CEILING:
-	roundingMode = (sign == 1) ? ROUND_UP : ROUND_DOWN;
-	break;
-      case ROUND_FLOOR:
-	roundingMode = (sign == 1) ? ROUND_DOWN : ROUND_UP;
-	break;
-      case ROUND_HALF_UP:
-	roundingMode = (roundDigit >= 5) ? ROUND_UP : ROUND_DOWN;
-	break;
-      case ROUND_HALF_DOWN:
-	roundingMode = (roundDigit > 5) ? ROUND_UP : ROUND_DOWN;
-	break;
-      case ROUND_HALF_EVEN:
-	if (roundDigit < 5)
-	  roundingMode = ROUND_DOWN;
-	else
+	// half is -1 if remainder*2 < positive intValue (*power), 0 if equal,
+	// 1 if >. This implies that the remainder to round is less than,
+	// equal to, or greater than half way to the next digit.
+	BigInteger posRemainder
+	  = parts[1].signum () < 0 ? parts[1].negate() : parts[1];
+	valIntVal = valIntVal.signum () < 0 ? valIntVal.negate () : valIntVal;
+	int half = posRemainder.shiftLeft(1).compareTo(valIntVal);
+
+	switch(roundingMode)
 	  {
-	    int rightmost = 
-	      unrounded.mod (BigInteger.valueOf (10)).intValue ();
-	    if (rightmost % 2 == 1) // odd, then ROUND_HALF_UP
+	  case ROUND_HALF_UP:
+	    roundingMode = (half < 0) ? ROUND_DOWN : ROUND_UP;
+	    break;
+	  case ROUND_HALF_DOWN:
+	    roundingMode = (half > 0) ? ROUND_UP : ROUND_DOWN;
+	    break;
+	  case ROUND_HALF_EVEN:
+	    if (half < 0)
+	      roundingMode = ROUND_DOWN;
+	    else if (half > 0)
+	      roundingMode = ROUND_UP;
+	    else if (unrounded.testBit(0)) // odd, then ROUND_HALF_UP
 	      roundingMode = ROUND_UP;
-	    else // even, then ROUND_HALF_DOWN
-	      roundingMode = (roundDigit > 5) ? ROUND_UP : ROUND_DOWN;
+	    else                           // even, ROUND_HALF_DOWN
+	      roundingMode = ROUND_DOWN;
+	    break;
 	  }
-	break;
       }
 
     if (roundingMode == ROUND_UP)
-      return new BigDecimal (unrounded.add (BigInteger.valueOf (1)), newScale);
+      unrounded = unrounded.add (BigInteger.valueOf (sign > 0 ? 1 : -1));
 
     // roundingMode == ROUND_DOWN
     return new BigDecimal (unrounded, newScale);