<i18n dev> RFR 8177552: Compact Number Formatting support

[naoto.sato at oracle.com]

Hi Nishit,

On 11/18/18 10:29 PM, Nishit Jain wrote:
> Hi Naoto,
> 
> Please check my comments inline.
> 
> On 17-11-2018 04:52, naoto.sato at oracle.com wrote:
>> Hi Nishit,
>>
>> Here are my comments:
>>
>> - CLDRConverter: As the compact pattern no more employs List<String>, 
>> can we eliminate stringListEntry/Element, and use Array equivalent 
>> instead?
> Since the CNF design does not put any limit on the size of compact 
> pattern, so at the time of parsing the CLDR xmls using SAX parser, it 
> becomes difficult to identify the size of array when the parent element 
> of compact pattern is encountered, so I think it is better to keep the 
> List<String> while extracting the resources.

OK. However I'd not keep the List<String> format on generating the 
resource bundle, as there is no reason to introduce yet another bundle 
format other than the existing array of String.

>>
>> - CompactNumberFormat.java
>>
>> Multiple locations: Use StringBuilder instead of StringBuffer.
> OK
>>
>> line 268: The link points to NumberFormat.getNumberInstance(Locale) 
>> instead of DecimalFormat
> OK. Changed it at line 165 also.
>>
>> line 855: no need to do toString(). length() can detect whether it's 
>> empty or not.
>>
>> line 884: "Overloaded method" reads odd here. I'd prefer specializing 
>> in the "given number" into either long or biginteger.
> OK
>>
>> line 1500: subparseNumber() pretty much shares the same code with 
>> DecimalFormat.subparse(). can they be merged?
> The existing CNF.subParseNumber differs in the way parseIntegerOnly is 
> handled, DecimalFormat.parse()/subparse() behaviour is unpredictable 
> with parseIntegeronly = true when multipliers are involved (Please see 
> JDK-8199223).
> 
> Also, I had thought that the CNF.parse()/subparseNumber() should *not 
> *parse the exponential notation e.g. while parsing "1.05E4K" the parsing 
> should break at 'E' and returns 1.05, because 'E' should be considered 
> as unparseable character for general number format pattern or compact 
> number pattern, but this is not the case with DecimalFormat.parse(). The 
> below DecimalFormat general number format instance
> 
> NumberFormat nf =  NumberFormat.getNumberInstance();
> nf.parse("1.05E4")
> 
> Successfully parse the string and returns 10500. The same behaviour is 
> there with other DecimalFormat instances also e.g. currency instance.
> 
> Do you think this is an issue with DecimalFormat.parse() and CNF should 
> avoid parsing exponential numbers? Or, should CNF.parse() be modified to 
> be consistent with DecimalFormat.parse() in this aspect?

No, I understand there are differences. But I see a lot of duplicated 
piece of code which I would like to eliminate.

> 
>>
>> line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls 
>> super. No need to override them.
> Since setters are overridden, I think that it is better to override 
> getters also (even if they are just calling super and have same javadoc) 
> to keep them at same level. But, if you see no point in keeping them in 
> CNF, I will remove them. Does that need CSR change?

I don't see any point for override. I don't think there needs a CSR, but 
better ask Joe about it.

>>
>> line 2231: You need to test the type before cast. Otherwise 
>> ClassCastException may be thrown.
> The type is checked in the superclass equals method getClass() != 
> obj.getClass(), so I think there is no need to check the type here.

OK.

Naoto

> 
> Regards,
> Nishit Jain
>>
>> Naoto
>>
>> On 11/16/18 9:54 AM, Nishit Jain wrote:
>>> Hi,
>>>
>>> Please review this non trivial feature addition to NumberFormat API.
>>>
>>> The existing NumberFormat API provides locale based support for 
>>> formatting and parsing numbers which includes formatting decimal, 
>>> percent, currency etc, but the support for formatting a number into a 
>>> human readable or compact form is missing. This RFE adds that feature 
>>> to format a decimal number in a compact format (e.g. 1000 -> 1K, 
>>> 1000000 -> 1M in en_US locale) , which is useful for the environment 
>>> where display space is limited, so that the formatted string can be 
>>> displayed in that limited space. It is defined by LDML's 
>>> specification for Compact Number Formats.
>>>
>>> http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats
>>>
>>>
>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
>>> Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8188147
>>>
>>> Request to please help review the the change.
>>>
>>> Regards,
>>> Nishit Jain
>>>
>>>
>>>
>>>
>