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.lagn...@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.lagn...@oracle.com wrote:
>> Could someone please review this code change ?
>>
>> Thanks in advance,
>> Olivier Lagneau
>>
>> On 09/09/2014 22:44, olivier.lagn...@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>
