Re: RFR(XS): 8075658: Mark intermittently failuring core-svc tests

[olivier.lagn...@oracle.com]

Hi Katja,

Looks ok to me too.



It has been a relatively manual process and I'm not aware of a 
mechanism how to sync test-key-bug. What I can do is to mark bugs I 
went through with for example 'key-intermittent' label to distinguish 
them form the new ones.
That's another additional label, and as I understand Joe's comment most 
important is to mark them intermittent with jtreg flag.

Don't have any other suggestions however.

Olivier.

On 17/07/2015 15:33, Yekaterina Kantserova wrote:

Jaroslav,

Thank you for the review!

It has been a relatively manual process and I'm not aware of a 
mechanism how to sync test-key-bug. What I can do is to mark bugs I 
went through with for example 'key-intermittent' label to distinguish 
them form the new ones.


Are there other suggestions?

// Katja



On 07/17/2015 03:04 PM, Jaroslav Bachorik wrote:

Looks good.

Is there any way to check that all the issues from the JBS query 
mentioned in JDK-8075658 are addressed?


-JB-

On 17.7.2015 14:47, Yekaterina Kantserova wrote:

Hi,

Could I please have a review of this fix.

bug: https://bugs.openjdk.java.net/browse/JDK-8075658
webrev: http://cr.openjdk.java.net/~ykantser/8075658/webrev.00

Thanks,
Katja

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

[olivier.lagn...@oracle.com]

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 



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

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

[olivier.lagn...@oracle.com]

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>


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

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

[olivier.lagn...@oracle.com]

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


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

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

[olivier.lagn...@oracle.com]

Hi William,

Thanks a lot for your time and checks !

On 19/09/2014 22:02, William Price wrote:

Hi Oliver,

First, sorry about mistyping your name, Olivier!


I copied your patch into my shim locally and ran my test cases and
still get a couple failures (see output below).  Your patch and my version
differ in the way and order in which we interpret allDecimalDigits and
alreadyRounded.

And I should have been more diligent. On second look these are all "very close to a 
tie"
as described in JDK-7131459, so I now believe my test suite is incorrect.  It 
matched the
JDK 6-7 behavior but did not account for the JDK8 intended changes for these 
edge cases.
Yes these are voluntarily all tricky cases with dtoa() returning an 
exact tie

due to IEEE-754 precision limit, thus following nearest to even policy and
either rounding up or truncating right at the rounding digit position.

allDecimalDigits indicates whether dtoa() was able to provide all the 
exact digits,
with result still inside IEEE-754 precision. alreadyRounded indicates 
whether the result

was truncated or rounded up.

I think allDecimalDigits must be checked first, followed by 
alreadyRounded, since
if you get an exact binary representation within IEEE precision, dtoa 
won't do

any rounding by principle.
I believe the latest (last Friday) version of your patch on github is ok.

Thanks again William for following this bug,
and for the feedback and checking of the patch for this review  !

Best Regards,
Olivier.



JDK-7131459 test case:
0.15 is actually: 
0.1499944488848768742172978818416595458984375
 HALF_UP  --> 0.1OK
0.35 is actually: 
0.34997779553950749686919152736663818359375
 HALF_UP  --> 0.3OK
0.85 is actually: 
0.84997779553950749686919152736663818359375
 HALF_UP  --> 0.8OK
0.95 is actually: 
0.9499555910790149937383830547332763671875
 HALF_UP  --> 0.9OK

Above tests used Java 1.8.0_20 (Oracle Corporation)
installed at C:\JAVA\latest8\jre

Agent installed: yes
Patch applied  : yes

Overall result : FIXED
  

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

[olivier.lagn...@oracle.com]

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 
<mailto: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 
<mailto: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 
<mailto: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><http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.00>

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

[olivier.lagn...@oracle.com]

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.lagn...@oracle.com 
<mailto:olivier.lagn...@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.68676607158436745521612465381622314453125

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

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

[olivier.lagn...@oracle.com]

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.lagn...@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.68676607158436745521612465381622314453125

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

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

[olivier.lagn...@oracle.com]

Please review this 2nd version of the fix taking into account your feedback.

Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.00 

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

[olivier.lagn...@oracle.com]

Please review this 2nd version of the fix taking into account your feedback.

Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.01 


I have changed the code following your remarks

Testing: lot of testing on jtreg, jck8, code coverage, jprt.

Below are more details on the changes in this webrev.

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 have merged HALF_UP and HALF_DOWN cases into a single switch case.

I also tested a fully merged version where HALF_UP, HALF_DOWN, and also 
HALF_EVEN are merge

into a single switch case. So merging the three is even possible.
If you want to have the three HALF_* cases we can quickly move to that.

My only suggestions are trivial: 1) enhance the method javadoc of 
shouldRoundUp(), e.g., there are no @param tags for the boolean 
parameters,

Fixed that. @param tags are available for shouldRoundUp method

2) use braces around all the statements contained in if/else blocks 
(see below). Comment #2 is nit-picky.

Took care to follow this rule in this new version.

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 …”

Changed test code to provide this consistent wording.

Thanks,
Olivier.

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

[olivier.lagn...@oracle.com]

Oops, please forget this one that is  missing text of what is done in 
this webrev.


wrong gui manipulation ...


Sorry for this,
Olivier.

On 02/10/2014 18:08, olivier.lagn...@oracle.com wrote:
Please review this 2nd version of the fix taking into account your 
feedback.


Bug : 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.01 
<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.00%20%3Chttp://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.01>

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

[olivier.lagn...@oracle.com]

Hi William,

On 07/10/2014 18:33, William Price wrote:

Please review this 2nd version of the fix taking into account your feedback.

I'm not recognized here as a reviewer, but the code looked OK to me and
it passed my patch test suite.
Thanks for letting me know. That provides additional confidence in the 
code !



I have merged HALF_UP and HALF_DOWN cases into a single switch case.

Good to know.  My current patch injects a method call to shim code
immediately after the HALF_UP case label.  I need to account for this or
else the applied patch could potentially cause HALF_DOWN to behave the
same as HALF_UP!  Thankfully, this is easily detectable in the bytecode
(not that the impact to my patch is of much concern to the core devs,
but it dove-tails nicely).

I'm looking forward to seeing this merged and the bug fixed/closed.  For my
company, the sooner this makes it into an 8u update, the better.

Sure.
This kind of code is very sensitive and we need to be very careful on 
the changes and during reviews.

That takes some time.
We expect to provide it soon in 8u base !

Thanks,
Olivier.

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

[olivier.lagn...@oracle.com]

Hi Joe,

Thanks for the relevant feedback.

I am going to provide all these changes in a new webrev within a few hours.

Thanks,
Olivier.

On 07/10/2014 18:25, Joe Darcy wrote:

Hi Olivier,

Some rewording suggestions:

 445  * @param alreadyRounded Boolean indicating if rounding up 
already happened.
 446  * @param allDecimalDigits Boolean indicating if the digits 
provide an exact

 447  * representation of the value.

Including the type of the parameter is redundant with its declaration 
so I'd recommend something like


 445  * @param alreadyRounded whether or not rounding up has 
already happened.


Given the semantics of allDecimalDigits, I recommend renaming the 
parameter to something like "valueExactAsDecimal".


For

please restore the conventional formatting

 532 } else if (digits[maximumDigits] == '5') {

The code

 543 if (roundingMode == 
RoundingMode.HALF_UP) {
 544 // Strictly follow HALF_UP rule 
==> round-up

 545 return true;
 546 }
 547 else {
 548 // Strictly follow HALF_DOWN rule 
==> don't round-up

 549 return false;
 550 }

can be replaced by

 545 return roundingMode == 
RoundingMode.HALF_UP;


with a suitable comment.

Please make these adjustments and I'll do a careful review of the 
rounding logic.


Thanks,

-Joe

On 10/2/2014 9:29 AM, olivier.lagn...@oracle.com wrote:
Please review this 2nd version of the fix taking into account your 
feedback.


Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.01 
<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.01>

I have changed the code following your remarks

Testing: lot of testing on jtreg, jck8, code coverage, jprt.

Below are more details on the changes in this webrev.

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 have merged HALF_UP and HALF_DOWN cases into a single switch case.

I also tested a fully merged version where HALF_UP, HALF_DOWN, and 
also HALF_EVEN are merge

into a single switch case. So merging the three is even possible.
If you want to have the three HALF_* cases we can quickly move to that.

My only suggestions are trivial: 1) enhance the method javadoc of 
shouldRoundUp(), e.g., there are no @param tags for the boolean 
parameters,

Fixed that. @param tags are available for shouldRoundUp method

2) use braces around all the statements contained in if/else blocks 
(see below). Comment #2 is nit-picky.

Took care to follow this rule in this new version.

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 …”

Changed test code to provide this consistent wording.

Thanks,
Olivier.

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

[olivier.lagn...@oracle.com]

Please review this 3rd version of the fix taking into account latest 
feedback from Joe.


Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
Webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.02/ 
<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.02/>


List of changes from previous webrev:
- renamed "allDecimalDigits" to "valueExactAsDecimal"
- changed related @param javadoc comments
- restored conventional formatting on "else if" clause
- changed rounding decision code when last digit at rounding position 
and valueExactAsDecimal true to synthetic return statement:

   "return roundingMode == RoundingMode.HALF_UP;" with adapted comment.

testing: jtreg, jck8, jprt, code coverage.

Thanks for reviewing,
Olivier.



On 07/10/2014 18:25, Joe Darcy wrote:

Hi Olivier,

Some rewording suggestions:

 445  * @param alreadyRounded Boolean indicating if rounding up 
already happened.
 446  * @param allDecimalDigits Boolean indicating if the digits 
provide an exact

 447  * representation of the value.

Including the type of the parameter is redundant with its declaration 
so I'd recommend something like


 445  * @param alreadyRounded whether or not rounding up has 
already happened.


Given the semantics of allDecimalDigits, I recommend renaming the 
parameter to something like "valueExactAsDecimal".


For

please restore the conventional formatting

 532 } else if (digits[maximumDigits] == '5') {

The code

 543 if (roundingMode == 
RoundingMode.HALF_UP) {
 544 // Strictly follow HALF_UP rule 
==> round-up

 545 return true;
 546 }
 547 else {
 548 // Strictly follow HALF_DOWN rule 
==> don't round-up

 549 return false;
 550 }

can be replaced by

 545 return roundingMode == 
RoundingMode.HALF_UP;


with a suitable comment.

Please make these adjustments and I'll do a careful review of the 
rounding logic.


Thanks,

-Joe

On 10/2/2014 9:29 AM, olivier.lagn...@oracle.com wrote:
Please review this 2nd version of the fix taking into account your 
feedback.


Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.01 
<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.01>

I have changed the code following your remarks

Testing: lot of testing on jtreg, jck8, code coverage, jprt.

Below are more details on the changes in this webrev.

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 have merged HALF_UP and HALF_DOWN cases into a single switch case.

I also tested a fully merged version where HALF_UP, HALF_DOWN, and 
also HALF_EVEN are merge

into a single switch case. So merging the three is even possible.
If you want to have the three HALF_* cases we can quickly move to that.

My only suggestions are trivial: 1) enhance the method javadoc of 
shouldRoundUp(), e.g., there are no @param tags for the boolean 
parameters,

Fixed that. @param tags are available for shouldRoundUp method

2) use braces around all the statements contained in if/else blocks 
(see below). Comment #2 is nit-picky.

Took care to follow this rule in this new version.

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 …”

Changed test code to provide this consistent wording.

Thanks,
Olivier.

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

[olivier.lagn...@oracle.com]

Thanks Joe,

Will modify and check the tests accordingly.

Brian, I guess you are still ok with these code changes ?

Thanks a lot both of you for the reviewing.

Thanks a lot William for checking with your own tests. That is very much 
appreciated !  ;-)


Olivier.


On 09/10/2014 19:16, Joe Darcy wrote:

Hi Olivier,

Please change the formatting any if-else statements like

 537 }
 538 else {

 545 }
 546 else {

 552 }
 553 else {

to be

 537 } else {

More substantively, these lines

 547 // Not an exact binary representation.
 548 if (alreadyRounded) {
 549 // Digit sequence rounded-up. Was 
below tie.

 550 // Don't round-up twice !
 551 return false;
 552 }
 553 else {
 554 // Digit sequence truncated. Was 
above tie.

 555 // must round-up !
 556 return true;
 557 }

Can be replaced by

return !alreadyRounded;

with appropriate commenting.

Please make these changes; as long as the tests still pass, no 
re-review is needed.


Thanks,

-Joe

On 10/9/2014 3:27 AM, olivier.lagn...@oracle.com wrote:
Please review this 3rd version of the fix taking into account latest 
feedback from Joe.


Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
Webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.02/ 
<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.02/>


List of changes from previous webrev:
- renamed "allDecimalDigits" to "valueExactAsDecimal"
- changed related @param javadoc comments
- restored conventional formatting on "else if" clause
- changed rounding decision code when last digit at rounding position 
and valueExactAsDecimal true to synthetic return statement:

   "return roundingMode == RoundingMode.HALF_UP;" with adapted comment.

testing: jtreg, jck8, jprt, code coverage.

Thanks for reviewing,
Olivier.



On 07/10/2014 18:25, Joe Darcy wrote:

Hi Olivier,

Some rewording suggestions:

 445  * @param alreadyRounded Boolean indicating if rounding up 
already happened.
 446  * @param allDecimalDigits Boolean indicating if the digits 
provide an exact

 447  * representation of the value.

Including the type of the parameter is redundant with its 
declaration so I'd recommend something like


 445  * @param alreadyRounded whether or not rounding up has 
already happened.


Given the semantics of allDecimalDigits, I recommend renaming the 
parameter to something like "valueExactAsDecimal".


For

please restore the conventional formatting

 532 } else if (digits[maximumDigits] == '5') {

The code

 543 if (roundingMode == 
RoundingMode.HALF_UP) {
 544 // Strictly follow HALF_UP rule 
==> round-up

 545 return true;
 546 }
 547 else {
 548 // Strictly follow HALF_DOWN 
rule ==> don't round-up

 549 return false;
 550 }

can be replaced by

 545 return roundingMode == 
RoundingMode.HALF_UP;


with a suitable comment.

Please make these adjustments and I'll do a careful review of the 
rounding logic.


Thanks,

-Joe

On 10/2/2014 9:29 AM, olivier.lagn...@oracle.com wrote:
Please review this 2nd version of the fix taking into account your 
feedback.


Bug : https://bugs.openjdk.java.net/browse/JDK-8039915
webrev : http://cr.openjdk.java.net/~olagneau/8039915/webrev.01 
<http://cr.openjdk.java.net/%7Eolagneau/8039915/webrev.01>

I have changed the code following your remarks

Testing: lot of testing on jtreg, jck8, code coverage, jprt.

Below are more details on the changes in this webrev.

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 have merged HALF_UP and HALF_DOWN cases into a single switch case.

I also tested a fully merged version where HALF_UP, HALF_DOWN, and 
also HALF_EVEN are merge

into a single switch case. So merging the three is even possible.
If you want to have the three HALF_* cases we can quickly move to that.

My only suggestions are trivial: 1) enhance the method javadoc of 
shouldRoundUp(), e.g., there are no @param tags for th

Sponsor needed : JDK-8039915 : Wrong NumberFormat.format() HALF_UP rounding when last digit exactly at rounding position greater than 5

[olivier.lagn...@oracle.com]

Could someone please sponsor this change for 9 ?

The webrev and patch are here:

http://cr.openjdk.java.net/~olagneau/8039915/webrev.03

And the review discussions are here:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-October/028956.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028583.html

The changeset comment is:

8039915: Wrong NumberFormat.format() HALF_UP rounding when last digit exactly 
at rounding position greater than 5
Summary: Fixes erroneous rounding in DigitList for corner cases uncovered 
previously. Adds dedicated unit tests to TieRoundingTest
Reviewed-by:bpb, darcy



Thanks,
Olivier

Re: DecimalFormat rounding changes in 8 (JEP 177)?

[olivier.lagn...@oracle.com]

Hello,

On 04/05/2014 20:41, Joe Darcy wrote:

Hello,

On 5/4/2014 9:56 AM, solo-java-core-l...@goeswhere.com wrote:

Hey,

Could someone please help me understand what changes have happened in
rounding in DecimalFormat in Java 8?

new DecimalFormat("0.00").format(1.035) is 1.04 on Java 7, and 1.03 on
Java 8.  (7u25-2.3.10-1~deb7u1, openjdk build 1.8.0_05-b13 debian and
Oracle build 1.8.0-b132 win64 tested).

My understanding of the RoundingMode.HALF_EVEN (the default)
documentation is that 1.04 is the correct answer.  In fact,
(0.000, 1.0035) is 1.004, and (0.0, 1.35) is 1.4.  I am aware
that floating point is more complex than this, and I am by no
means an expert.


There are several inexact processes going on here. The first is the 
decimal to binary conversion of 1.035 to a binary double value. In 
general, decimal fractions are not exactly representable in binary. 
Java's semantics require that on decimal to binary conversion, the 
double value with a numerical value closest to the exact value be 
used, the round to nearest even policy.


The exact numerical value of the double closest to 1.035 is

1.0349200639422269887290894985198974609375

a value slightly *less than* 1.035. When this value is rounded to two 
digits after the decimal point using the round to nearest even 
rounding mode, the numerically correct answer is 1.03 *not* 1.04.


A range of numbers of the real line will get converted to a particular 
floating-point value. Some of these ranges straddle half-way points in 
decimal. For example, the range of values that will get converted to 
the floating-point value in question includes


[1.0349200639422269887290894985198974609375, 1.035]

The full range is

[1.03480904163976447307504713535308837890625,
 1.03503108624468950438313186168670654296875]

This range is a one-ulp (unit in the last place, see Math.ulp) wide 
region centered on the exact floating-point value.


When a decimal to binary conversion occurs, the original decimal text 
value is lost. Therefore, after the conversion, the binary double 
value doesn't "know" it came from "1.035" or "1.03481" or 
something else.


The numerically correct behavior is the new behavior in JDK 8.

HTH,

-Joe

Just to insist on Joe's words (Thanks Joe for the detailed reply):
with floating-point "What You See *Is Not* What You Get" in most cases,
and this is true with DecimalFormat when formatting double or float values.

Don't expect exactness with floating-point. This is even true with constant
values inside you source code like 1.035 here (which cannot be represented
exactly in binary format).

JDK8 fixes a bug that was discovered during JDK7 dev.

Hope that helps,
Olivier.







It seems that this may be new code, with a known breaking change in it:

http://openjdk.java.net/jeps/177:

Compatibility: On JDK 7, (correct) but altered numerical behavior will
only be enabled under an aggressive optimization flag to limit
behavioral compatibility impact in that release train. In Java SE 8,
the correct numerical behavior will always be required by the
specification.

Did this materialise somewhere, and, if so, where's it documented?


In summary: My tests fail on Java 8 and I'm not sure they're wrong.
Any help would be appreciated, thanks.

Re: DecimalFormat rounding changes in 8 (JEP 177)?

[olivier.lagn...@oracle.com]

On 05/05/2014 09:59, Bernd wrote:

Hello,

Can you maybe point to the commit or Bug Number for this? The outcome of
this correctness fit is pretty unfortunate (at least for the Number in
question).

Initial bug Number in JDK7 is JDK-7131459:
https://bugs.openjdk.java.net/browse/JDK-7131459

related JDK8 bug is :
https://bugs.openjdk.java.net/browse/JDK-8000978

Hth,
Olivier


I could imagine a new RoundingMode could help for users which insist on
convieningly shoot themself in the foot.

Greetings
Bernd
Hello,

On 04/05/2014 20:41, Joe Darcy wrote:


Hello,

On 5/4/2014 9:56 AM, solo-java-core-l...@goeswhere.com wrote:


Hey,

Could someone please help me understand what changes have happened in
rounding in DecimalFormat in Java 8?

new DecimalFormat("0.00").format(1.035) is 1.04 on Java 7, and 1.03 on
Java 8.  (7u25-2.3.10-1~deb7u1, openjdk build 1.8.0_05-b13 debian and
Oracle build 1.8.0-b132 win64 tested).

My understanding of the RoundingMode.HALF_EVEN (the default)
documentation is that 1.04 is the correct answer.  In fact,
(0.000, 1.0035) is 1.004, and (0.0, 1.35) is 1.4.  I am aware
that floating point is more complex than this, and I am by no
means an expert.


There are several inexact processes going on here. The first is the
decimal to binary conversion of 1.035 to a binary double value. In general,
decimal fractions are not exactly representable in binary. Java's semantics
require that on decimal to binary conversion, the double value with a
numerical value closest to the exact value be used, the round to nearest
even policy.

The exact numerical value of the double closest to 1.035 is

 1.0349200639422269887290894985198974609375

a value slightly *less than* 1.035. When this value is rounded to two
digits after the decimal point using the round to nearest even rounding
mode, the numerically correct answer is 1.03 *not* 1.04.

A range of numbers of the real line will get converted to a particular
floating-point value. Some of these ranges straddle half-way points in
decimal. For example, the range of values that will get converted to the
floating-point value in question includes

[1.0349200639422269887290894985198974609375, 1.035]

The full range is

[1.03480904163976447307504713535308837890625,
  1.03503108624468950438313186168670654296875]

This range is a one-ulp (unit in the last place, see Math.ulp) wide region
centered on the exact floating-point value.

When a decimal to binary conversion occurs, the original decimal text
value is lost. Therefore, after the conversion, the binary double value
doesn't "know" it came from "1.035" or "1.03481" or something
else.

The numerically correct behavior is the new behavior in JDK 8.

HTH,

-Joe


Just to insist on Joe's words (Thanks Joe for the detailed reply):
with floating-point "What You See *Is Not* What You Get" in most cases,
and this is true with DecimalFormat when formatting double or float values.

Don't expect exactness with floating-point. This is even true with constant
values inside you source code like 1.035 here (which cannot be represented
exactly in binary format).

JDK8 fixes a bug that was discovered during JDK7 dev.

Hope that helps,
Olivier.




It seems that this may be new code, with a known breaking change in it:

http://openjdk.java.net/jeps/177:


Compatibility: On JDK 7, (correct) but altered numerical behavior will
only be enabled under an aggressive optimization flag to limit
behavioral compatibility impact in that release train. In Java SE 8,
the correct numerical behavior will always be required by the
specification.


Did this materialise somewhere, and, if so, where's it documented?


In summary: My tests fail on Java 8 and I'm not sure they're wrong.
Any help would be appreciated, thanks.