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

[olivier.lagneau at 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.lagneau at 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.
>>
>>
>>
>>
>