<i18n dev> RFR 8177552: Compact Number Formatting support
naoto.sato at oracle.com
naoto.sato at oracle.com
Mon Nov 19 19:03:36 UTC 2018
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
>>>
>>>
>>>
>>>
>
More information about the core-libs-dev
mailing list