RFR: 8251499: no-placeholder compact number patterns throw IllegalArgumentException

[Roger Riggs]

Hi Naoto,

CompactNumberFormat.java:

269: The field "placeHolders" should be named consistently with the 
other holders of Patterns.
   -> placeHolderPatterns

1632: missing space before ":"

2390:  I'm not sure why the stream processing is preferable to a direct 
forEach(...).

TestCompactPatternsValidity:
For the Plurals cases it would clearer to create a new Data provider and 
corresponding tests
with the extra arg.

Thanks, Roger



On 8/18/20 5:13 PM, Joe Wang wrote:
> Hi Naoto,
>
> That's nice!  The change looks good to me.
>
> Regards,
> Joe
>
> On 8/18/20 11:37 AM, naoto.sato at oracle.com wrote:
>> Hi Joe,
>>
>> Thank you for your comment. I consolidated those duplicated pieces 
>> into one piece. Did not make it a private method, though, as it would 
>> need to return two results from the method (cnfMultiplier and whether 
>> to return immediately for no-placeholder cases).
>>
>> https://cr.openjdk.java.net/~naoto/8251499/webrev.02/
>>
>> Naoto
>>
>> On 8/17/20 11:52 PM, Joe Wang wrote:
>>> Hi Naoto,
>>>
>>> Looks good overall. One nit, blocks 1633-1639 and 1642-1649 may 
>>> share a common private method with a parameter that takes either 
>>> matchedPosIndex or matchedNegIndex, if you want.
>>>
>>> Best,
>>> Joe
>>>
>>> On 8/17/20 4:42 PM, naoto.sato at oracle.com wrote:
>>>> Hi Joe,
>>>>
>>>> It turned out that the previous fix did not address plural format 
>>>> cases. That means that just making the divisor negative to indicate 
>>>> non-placeholder cannot distinguish multiple plural cases with the 
>>>> same divisor. Instead, I created a list of placeholders (minimum 
>>>> digits) for each index and count. Here is the updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~naoto/8251499/webrev.01/
>>>>
>>>> I added a new test case (COMPACT_PATTERN14), which actually is 
>>>> extracted from CLDR 38 Somali locale that demonstrates the issue. 
>>>> I'd appreciate your further review.
>>>>
>>>> Naoto
>>>>
>>>> On 8/14/20 6:21 PM, Joe Wang wrote:
>>>>> Hi Naoto,
>>>>>
>>>>> Looks good to me.
>>>>>
>>>>> While a negative divisor representing no zeros is newly 
>>>>> introduced, the "divisor > 0" checks seem to have always been 
>>>>> beneficial.  I had to count the number of ""s in COMPACT_PATTERN13 
>>>>> :-)
>>>>>
>>>>> Have a great weekend!
>>>>> Joe
>>>>>
>>>>> On 8/14/2020 3:20 PM, naoto.sato at oracle.com wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the fix for the following issue:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8251499
>>>>>>
>>>>>> The proposed changeset is located at:
>>>>>>
>>>>>> https://cr.openjdk.java.net/~naoto/8251499/webrev.00/
>>>>>>
>>>>>> The current implementation of CompactNumberFormat assumes that 
>>>>>> there is always the number placeholder part in compact patterns. 
>>>>>> This is not always true. In fact, upcoming CLDR 38 resurrects 
>>>>>> such patterns, so this fix is a precursor to support CLDR 38.
>>>>>>
>>>>>> Naoto
>>>>>
>>>
>