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

[Roger Riggs]

Hi Naoto,

Looks good, thanks for the updates.

Roger


On 8/19/20 2:26 PM, naoto.sato at oracle.com wrote:
> Hi Roger,
>
> Thank you for your comments. I've addressed your suggestions and 
> created a new webrev below:
>
> https://cr.openjdk.java.net/~naoto/8251499/webrev.03/
>
> Naoto
>
> On 8/19/20 8:33 AM, Roger Riggs wrote:
>> 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
>>>>>>>
>>>>>
>>>
>>