Hi Naoto,

That's nice!  The change looks good to me.

Regards,
Joe

On 8/18/20 11:37 AM, naoto.s...@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.s...@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.s...@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



Reply via email to