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

[Joe Darcy]

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