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

[Mark Wielaard <mark at klomp dot org>]

Hi,

On Tue, 2003-07-01 at 08:03, Per Bothner wrote:
> > Hey, folks.  Could someone take at this?  I had posted a modified
> > patch for fixing the above in
> > http://gcc.gnu.org/ml/java-patches/2003-q2/msg00392.html

Since I had the same problem/bugs with the GNU Classpath code (which is
the upstream of the libgcj code) I took a look at this. And added some
more tests for various corner cases to Mauve.

> The multiplication by two in the calculation of half would be more
> efficient as a shiftLeft(1).

Done. But see below.

> It probaby would be better to avoid calculating half when it is not
> needed.  That does make the logic slightly uglier.  Perhaps:
> 
> 
>      switch (roundingMode)
>        {
>        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_UP:  case ROUND_DOWN:
>          break;
>        default:
>          half = ...;
>          switch (roundMode)
>            ...
>       }

You need half also to see if ROUND_UNNECESSARY can be satisfied.
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.

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.

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

> More generally, have you considered what happens when val
> is negative?  The code might work - or it might now.  I haven't
> tried to analyze what happens.

There was something going wrong in some cases.
Since I found it hard to analyze what really went on I simplified the
code a bit by calculating half by using the positive remainder and then
make sure that in the final ROUND_UP case we just use the sign (or 1 if
the sign is positive) to get further away from 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.

07-18-2003  Jerry Quinn  <jlquinn@optonline.net>
            Mark Wielaard  <mark@klomp.org>

        * java/math/BigDecimal (divide): Correctly handle
        ROUND_HALF_EVEN when amount is greater than 0.5.
	Simplify code.

OK to commit?

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	18 Jul 2003 10:49:35 -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
@@ -288,47 +288,51 @@
 //      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);
 
+    if (roundingMode == ROUND_UNNECESSARY)
+      throw new ArithmeticException ("newScale is not large enough");
+
     int sign = unrounded.signum ();
 
-    switch (roundingMode)
+    if (roundingMode == ROUND_CEILING)
+      roundingMode = (sign == 1) ? ROUND_UP : ROUND_DOWN;
+    else if (roundingMode == ROUND_FLOOR)
+      roundingMode = (sign == 1) ? ROUND_DOWN : ROUND_UP;
+    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 = sign == -1 ? parts[1].negate() : parts[1];
+	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 == -1) ? ROUND_DOWN : ROUND_UP;
+	    break;
+	  case ROUND_HALF_DOWN:
+	    roundingMode = (half == 1) ? ROUND_UP : ROUND_DOWN;
+	    break;
+	  case ROUND_HALF_EVEN:
+	    if (half == -1)
+	      roundingMode = ROUND_DOWN;
+	    else if (half == 1)
+	      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);
+      return new BigDecimal (unrounded.add (BigInteger.valueOf
+					    (sign != 0 ? sign : 1)), newScale);
 
     // roundingMode == ROUND_DOWN
     return new BigDecimal (unrounded, newScale);