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

[Joe Darcy]

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