Urgent [9] RFR (S) : JDK-8039915 NumberFormat.format() does not consider required no. of fraction digits properly

[Brian Burkhalter]

Hello Olivier,

By inspection I think that the fix and the test update look good. I verified that the test hits all five branches contained in the else-if block of DigitList beginning at line 527. I also verified that the current code base fails four test cases when run against the updated test. The welcome testing performed by William Price is good corroboration as well.

My only suggestions are trivial: 1) enhance the method javadoc of shouldRoundUp(), e.g., there are no @param tags for the boolean parameters, and 2) use braces around all the statements contained in if/else blocks (see below). Comment #2 is nit-picky.

Lastly and this is not really part of your changeset, but I found it curious that the test prints the details of all cases that succeed as opposed to those that fail. I would think that either all results or the failures alone ought to be printed instead of successes only. See for example the partial diff below the DigitList diff.

Thanks,

Brian

--- a/src/java.base/share/classes/java/text/DigitList.java
+++ b/src/java.base/share/classes/java/text/DigitList.java
@@ -526,24 +526,25 @@
                 }
                 else if (digits[maximumDigits] == '5') {
                     // Digit at rounding position is a '5'. Tie cases.
-                    if (maximumDigits != (count - 1))
+                    if (maximumDigits != (count - 1)) {
                         // Remaining digits. Above tie  => must round-up
                         return true;
-                    else {
+                    } else {
                         // Digit at rounding position is the last one !
-                        if (allDecimalDigits)
+                        if (allDecimalDigits) {
                             // Exact binary representation. On the tie.
                             // Strictly follow HALF_UP rule ==> round-up
                             return true;
-                        else {
-                            if (alreadyRounded)
+                        } else {
+                            if (alreadyRounded) {
                                 // Digit sequence rounded-up. Was below tie.
                                 // Don't round-up twice !
                                 return false;
-                            else
+                            } else {
                                 // Digit sequence truncated. Was above tie
                                 // HALF_UP rule ==>  must round-up !
                                 return true;
+                            }
                         }
                     }
                 }

test/java/text/Format/DecimalFormat/TieRoundingTest.java

             errorCounter++;
             allPassed = false;
+            System.out.println("\nFailure for double value : " + doubleToTest + " :");
         } else {
             testCounter++;
             System.out.println("\nSuccess for double value : " + doubleToTest + " :");
-            System.out.println(" Input digits :" + inputDigits +
-                               ", BigDecimal value : " +
-                               new BigDecimal(doubleToTest).toString());
-            System.out.print(" Rounding mode: " + rm);
-            System.out.print(", fract digits : " + mfd);
-            System.out.print(", position : " + tiePosition + " tie");
-            System.out.print(", result : " + result);
-            System.out.println(", expected : " + expectedOutput);
         }
+
+        System.out.println(" Input digits :" + inputDigits
+                + ", BigDecimal value : "
+                + new BigDecimal(doubleToTest).toString());
+        System.out.print(" Rounding mode: " + rm);
+        System.out.print(", fract digits : " + mfd);
+        System.out.print(", position : " + tiePosition + " tie");
+        System.out.print(", result : " + result);
+        System.out.println(", expected : " + expectedOutput);
     }

On Sep 19, 2014, at 3:31 AM, olivier.lagneau at oracle.com wrote:

> This is a code change we would like to integrate in next release of JDK8, since impacting some customer apps/deployments.
> So it would be good to have it reviewed and backported to 8 soon.
> 
> Is there anyone having a bit of free time to review it ?
> 
> Best Regards,
> Olivier Lagneau
> 
> 
> On 11/09/2014 18:07, olivier.lagneau at oracle.com wrote:
>> Could someone please review this code change ?
>> 
>> Thanks in advance,
>> Olivier Lagneau
>> 
>> On 09/09/2014 22:44, olivier.lagneau at oracle.com wrote:
>>> Please review this fix in for wrong rounding-mode mode behavior of NumberFormat.format(double) in HALF_UP case.
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8039915
>>> 
>>> webrev: http://cr.openjdk.java.net/~olagneau/8039915/webrev.00<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.00>