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

[olivier.lagneau at oracle.com]

Hi Joe,

Thanks a lot for taking the time to review !

Please see comments inlined.

On 23/09/2014 04:51, Joe Darcy wrote:
> Hello,
>
> I've looked over the proposed changeset as well.
>
> I don't see a problem with the code, but I'm not (yet) convinced it is 
> right.
>
> For future work, I think be clearer to combine the CEILING and FLOOR 
> cases to share a loop with a condition test based on CEILING/FLOOR lie.
Agreed.
> In the test, again for future work, I think it would be clearer to 
> create a custom class to represent the tuple of information needed for 
> a test case as opposed to spreading that information about over a set 
> of parallel arrays.
Sure. With this set of separate independent arrays, the test is 
currently not much readable, and is weak with regards of maintenance
and evolutivity.
>
> 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 think so too. I even think it might be good to group HALF_EVEN with 
those two, since behaviour of dtoa impacts
these 3 cases in the same manner when very close to tie.

Will change the code to group HALF_UP and HALF_DOWN.

Thanks,
Olivier.

>
> Thanks,
>
> -Joe
>
> On 09/22/2014 08:50 AM, Brian Burkhalter wrote:
>> Hi Olivier,
>>
>> On Sep 22, 2014, at 7:56 AM, olivier.lagneau at oracle.com wrote:
>>
>>>> and 2) use braces around all the statements contained in if/else 
>>>> blocks (see below). Comment #2 is nit-picky.
>>> I tried to keep the same flavour of writing as in HALF_DOWN and 
>>> HALF_EVEN cases, i.e. don't use brace for
>>> terminal/leaf return true/false statements. This is not the standard 
>>> however, at least in this file.
>>> Will use braces in all case (i.e. the 3 of HALF_UP,  HALF_DOWN and 
>>> HALF_EVEN).
>> I did not look at the other case. If your formatting matches the rest 
>> of the file then I think it is OK to leave it as-is.
>>
>>>> Lastly and this is not really part of your changeset, but I found 
>>>> it curious that the test prints the details of all cases that 
>>>> succeed as opposed to those that fail. I would think that either 
>>>> all results or the failures alone ought to be printed instead of 
>>>> successes only. See for example the partial diff below the 
>>>> DigitList diff.
>>> Since these are most often corner and tricky test cases I think it 
>>> interesting to have the details of each result,
>>> including infos of both why returned result is correct or wrong.
>>> That can help the reader to understand all these tricky cases.
>>> The bad side of it being that it prints a lot of text, with failure 
>>> cases (hoepfully few) lost in the middle of it,
>>> thus making failures possibly not immediate to detect.
>>>
>>> Here is an example of what is printed in case of failure:
>>> "
>>> ========================================
>>> ***Error formatting double value from string : 0.6868d
>>> NumberFormat pattern is  : #,##0.###
>>> Maximum number of fractional digits : 3
>>> Fractional rounding digit : 4
>>> Position of value relative to tie : above
>>> Rounding Mode : HALF_UP
>>> BigDecimal output : 
>>> 0.68679999999999996607158436745521612465381622314453125
>>> FloatingDecimal output : 0.6868
>>> Error. Formatted result different from expected.
>>> Expected output is : "0.687"
>>> Formated output is : "0.686"
>>> ========================================
>> I missed that output: I was looking for the word “failure.”
>>
>>> There is also a reminder of the number of errors at the end of the 
>>> report:
>>> "
>>> ==> 4 tests failed
>>>
>>> Test failed with 4 formating error(s).
>>> "
>>>
>>> May be providing a reminder (value + rounding-mode + rounding position)
>>> of the failure cases at the end of the would be better ?
>>> Like:
>>> "
>>> Test failed with 4 formating error(s) for following cases :
>>> - 0.3126d, HALF_UP rounding, 3 fractional digits
>>> - 0.6868d, HALF_UP rounding, 3 fractional digits
>>> - 1.798876d, HALF_UP rounding, 5 fractional digits
>>> - 1.796889d, HALF_UP rounding, 5 fractional digits
>>> "
>>>
>>> Would doing so be ok ?
>> 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 …”
>>
>> Thanks,
>>
>> Brian
>