Hi Naoto,
Looks good, thanks for the updates.
Roger
On 8/19/20 2:26 PM, naoto.s...@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.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
