Urgent [9] RFR (S) : JDK-8039915 NumberFormat.format() does not consider required no. of fraction digits properly
olivier.lagneau at oracle.com
olivier.lagneau at oracle.com
Mon Sep 22 14:56:49 UTC 2014
Hi Brian,
Thanks a lot for your thorough review of the fix !
Please see comments inlined.
On 20/09/2014 00:50, Brian Burkhalter wrote:
> 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.
Thanks for checking all these. with details of
>
> My only suggestions are trivial: 1) enhance the method javadoc of
> shouldRoundUp(), e.g., there are no @param tags for the boolean
> parameters,
Did not notice that. Will add those.
> and 2) use braces around all the statements contained in if/else
> blocks (see below). Comment #2 is nit-picky.
I tried to keep the same flavour of writing as in HALF_DOWN and
HALF_EVEN cases, i.e. don't use brace for
terminal/leaf return true/false statements. This is not the standard
however, at least in this file.
Will use braces in all case (i.e. the 3 of HALF_UP, HALF_DOWN and
HALF_EVEN).
>
> 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.
Since these are most often corner and tricky test cases I think it
interesting to have the details of each result,
including infos of both why returned result is correct or wrong.
That can help the reader to understand all these tricky cases.
The bad side of it being that it prints a lot of text, with failure
cases (hoepfully few) lost in the middle of it,
thus making failures possibly not immediate to detect.
Here is an example of what is printed in case of failure:
"
========================================
***Error formatting double value from string : 0.6868d
NumberFormat pattern is : #,##0.###
Maximum number of fractional digits : 3
Fractional rounding digit : 4
Position of value relative to tie : above
Rounding Mode : HALF_UP
BigDecimal output : 0.68679999999999996607158436745521612465381622314453125
FloatingDecimal output : 0.6868
Error. Formatted result different from expected.
Expected output is : "0.687"
Formated output is : "0.686"
========================================
"
There is also a reminder of the number of errors at the end of the report:
"
==> 4 tests failed
Test failed with 4 formating error(s).
"
May be providing a reminder (value + rounding-mode + rounding position)
of the failure cases at the end of the would be better ?
Like:
"
Test failed with 4 formating error(s) for following cases :
- 0.3126d, HALF_UP rounding, 3 fractional digits
- 0.6868d, HALF_UP rounding, 3 fractional digits
- 1.798876d, HALF_UP rounding, 5 fractional digits
- 1.796889d, HALF_UP rounding, 5 fractional digits
"
Would doing so be ok ?
Best Regards,
Olivier.
>
> 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
> <mailto: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
>> <mailto: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
>>> <mailto: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><http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.00>
>
More information about the core-libs-dev
mailing list