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

[Per Bothner <per at bothner dot com>]

Jerry Quinn 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

Sorry for letting this slide.

Here are my comments from looking at the code.

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

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

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.

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.  Have you tested it?
--
	--Per Bothner
per@bothner.com   http://per.bothner.com/