Urgent [9], 2nd round, 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
Thu Oct 2 16:29:31 UTC 2014


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.







More information about the core-libs-dev mailing list