patch ping Re: [java] Fix BigDecimal ROUND_HALF_EVEN

[Mark Wielaard]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: BigDecimal.patch
Type: text/x-patch
Size: 3877 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/java/attachments/20030718/cd308dc3/attachment.bin>