<i18n dev> RFR 8177552: Compact Number Formatting support

[Nishit Jain]

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

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

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