[14] RFR: 8222756: Plural support in CompactNumberFormat

Roger Riggs Roger.Riggs at oracle.com
Wed Dec 4 20:16:40 UTC 2019


Hi Naoto,

Looks very good, and a few comments...


CompactNumberFormat.java:

144: "each singular and plural patterns" -> "each singular and plural 
pattern"

148: "Should the pattern include" -> "If the pattern includes"

336:  Add a "." at the end of the sentence

407/417:  If the plural rules is not syntatically correct, how is the 
error reported?  IllegalArgumentException?

587-588: Can be simplified a bit:
     The pattern appears multiple times and could be a utility method.

     private String getPrefix(boolean isNegative, int compactDataIndex, 
int iPart) {
         return (isNegative ? negativePrefixPatterns : 
positivePrefixPatterns)
                 .get(compactDataIndex).get(iPart);
     }

1667: Pattern DIGITS; is there a pattern syntax to surround the inf and 
nan parts
to ensure they are treated as literals?
Can Pattern.compile throw? The parse(text,pos) method does not declare 
IllegalArgumentException.
It might be clearer to check for inf/nan outside the digits parse pattern.

2378: Using == on doubles seems like a risky choice but may be ok given
that the input is integers, not floating point.

2350-2353 Looks ok like a lot of pattern matching on every parse.
If performance turns out to be an issue, it should be possible to parse
and generate a tree of lambdas with bindings to the literal values.
The lambda expressions could be saved and re-used.


TestEquality.java:

51: please break the line into reasonable line lengths.

Generated PluralRules.java:  Each plural entry has an extra space at the 
end of the string;
CLDRConverter.java:
1215: Please trim strings to avoid looking like they are significant.

Regards, Roger


ResourceBundleGenerator.java does not have any changes and may not need 
to be in the webrev.

On 11/26/19 4:35 PM, naoto.sato at oracle.com wrote:
> I modified CompactNumberFormat.java to simplify the syntax parsing:
>
> https://cr.openjdk.java.net/~naoto/8222756/webrev.01/
>
> Please review this webrev instead.
>
> Naoto
>
> On 11/25/19 1:16 PM, naoto.sato at oracle.com wrote:
>> Hello,
>>
>> CompactNumberFormat has been added since JDK 12 to support compact 
>> number formatting, such as 10_000 being formatted as "10K." [1] It 
>> works fine for English, however, not for other languages that take 
>> plural forms in formatted number prefixes/suffixes. In order to fix 
>> this, I filed the following CSR to extend the current 
>> CompactNumberFormat spec:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8232633
>>
>> It basically accommodates the plural prefix/suffix forms into the 
>> existing compact patterns array, so that the existing compact number 
>> format works compatibly. The plural rules are solely based on the 
>> CLDR's plural language rules [2]
>>
>> Here is the implementation of the CSR:
>>
>> https://cr.openjdk.java.net/~naoto/8222756/webrev.00/
>>
>> Please review the CSR as well as its implementation.
>>
>> Naoto
>>
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8177552
>> [2] 
>> https://unicode.org/reports/tr35/tr35-numbers.html#Language_Plural_Rules



More information about the core-libs-dev mailing list