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

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