[9] RFR (S) : JDK-8039915 NumberFormat.format() does not consider required no. of fraction digits properly
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> Bug came from a mix of a '5' and 'greater than 5' last digit, when this digit was the last one. Fix consists in properly separating the two cases and cleanly test all possible cases (see JDK-8039915 comments) for the HALF_UP rounding-mode. TieRoundingTest test has been extended with a few cases to check such cases (last digit at the rounding position and != '5') testing: jtreg, jck8, jprt Best regards, Olivier Lagneau
Could someone please review this code change ? Thanks in advance, Olivier Lagneau On 09/09/2014 22:44, olivier.lagneau@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>
Bug came from a mix of a '5' and 'greater than 5' last digit, when this digit was the last one.
Fix consists in properly separating the two cases and cleanly test all possible cases (see JDK-8039915 comments) for the HALF_UP rounding-mode.
TieRoundingTest test has been extended with a few cases to check such cases (last digit at the rounding position and != '5')
testing: jtreg, jck8, jprt
Best regards, Olivier Lagneau
Hi, 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@oracle.com wrote:
Could someone please review this code change ?
Thanks in advance, Olivier Lagneau
On 09/09/2014 22:44, olivier.lagneau@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>
Bug came from a mix of a '5' and 'greater than 5' last digit, when this digit was the last one.
Fix consists in properly separating the two cases and cleanly test all possible cases (see JDK-8039915 comments) for the HALF_UP rounding-mode.
TieRoundingTest test has been extended with a few cases to check such cases (last digit at the rounding position and != '5')
testing: jtreg, jck8, jprt
Best regards, Olivier Lagneau
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@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@oracle.com wrote:
Could someone please review this code change ?
Thanks in advance, Olivier Lagneau
On 09/09/2014 22:44, olivier.lagneau@oracle.com wrote:
Please review this fix in for wrong rounding-mode mode behavior of NumberFormat.format(double) in HALF_UP case.
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@oracle.com <mailto:olivier.lagneau@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@oracle.com <mailto:olivier.lagneau@oracle.com>wrote:
Could someone please review this code change ?
Thanks in advance, Olivier Lagneau
On 09/09/2014 22:44,olivier.lagneau@oracle.com <mailto:olivier.lagneau@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>
Hi Olivier, On Sep 22, 2014, at 7:56 AM, olivier.lagneau@oracle.com wrote:
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).
I did not look at the other case. If your formatting matches the rest of the file then I think it is OK to leave it as-is.
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" ========================================
I missed that output: I was looking for the word “failure.”
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 ?
If the test is already printing out the information you showed above (“Error formatting …”) then I think it is enough but the verbiage should perhaps match the reminder, e.g., “Failure: Error formatting double …” Thanks, Brian
Hello, I've looked over the proposed changeset as well. I don't see a problem with the code, but I'm not (yet) convinced it is right. For future work, I think be clearer to combine the CEILING and FLOOR cases to share a loop with a condition test based on CEILING/FLOOR lie. In the test, again for future work, I think it would be clearer to create a custom class to represent the tuple of information needed for a test case as opposed to spreading that information about over a set of parallel arrays. For the new work in question, it might be clearer to me if the HALF_UP and HALF_DOWN cases were combined into a single block since they share much of the logic. The unique logic for each mode would be easier to see if the differences were placed together. Thanks, -Joe On 09/22/2014 08:50 AM, Brian Burkhalter wrote:
Hi Olivier,
On Sep 22, 2014, at 7:56 AM, olivier.lagneau@oracle.com wrote:
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). I did not look at the other case. If your formatting matches the rest of the file then I think it is OK to leave it as-is.
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" ======================================== I missed that output: I was looking for the word “failure.”
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 ? If the test is already printing out the information you showed above (“Error formatting …”) then I think it is enough but the verbiage should perhaps match the reminder, e.g., “Failure: Error formatting double …”
Thanks,
Brian
Hi Joe, Thanks a lot for taking the time to review ! Please see comments inlined. On 23/09/2014 04:51, Joe Darcy wrote:
Hello,
I've looked over the proposed changeset as well.
I don't see a problem with the code, but I'm not (yet) convinced it is right.
For future work, I think be clearer to combine the CEILING and FLOOR cases to share a loop with a condition test based on CEILING/FLOOR lie. Agreed. In the test, again for future work, I think it would be clearer to create a custom class to represent the tuple of information needed for a test case as opposed to spreading that information about over a set of parallel arrays. Sure. With this set of separate independent arrays, the test is currently not much readable, and is weak with regards of maintenance and evolutivity.
For the new work in question, it might be clearer to me if the HALF_UP and HALF_DOWN cases were combined into a single block since they share much of the logic. The unique logic for each mode would be easier to see if the differences were placed together. I think so too. I even think it might be good to group HALF_EVEN with those two, since behaviour of dtoa impacts these 3 cases in the same manner when very close to tie.
Will change the code to group HALF_UP and HALF_DOWN. Thanks, Olivier.
Thanks,
-Joe
On 09/22/2014 08:50 AM, Brian Burkhalter wrote:
Hi Olivier,
On Sep 22, 2014, at 7:56 AM, olivier.lagneau@oracle.com wrote:
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). I did not look at the other case. If your formatting matches the rest of the file then I think it is OK to leave it as-is.
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" ======================================== I missed that output: I was looking for the word “failure.”
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 ? If the test is already printing out the information you showed above (“Error formatting …”) then I think it is enough but the verbiage should perhaps match the reminder, e.g., “Failure: Error formatting double …”
Thanks,
Brian
Thanks Brian, Will take into account your wording remarks. Olivier. On 22/09/2014 17:50, Brian Burkhalter wrote:
Hi Olivier,
On Sep 22, 2014, at 7:56 AM, olivier.lagneau@oracle.com <mailto:olivier.lagneau@oracle.com> wrote:
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).
I did not look at the other case. If your formatting matches the rest of the file then I think it is OK to leave it as-is.
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" ========================================
I missed that output: I was looking for the word “failure.”
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 ?
If the test is already printing out the information you showed above (“Error formatting …”) then I think it is enough but the verbiage should perhaps match the reminder, e.g., “Failure: Error formatting double …”
Thanks,
Brian
participants (3)
-
Brian Burkhalter
-
Joe Darcy
-
olivier.lagneau＠oracle.com