Re: [13] RFR 8217254, 8217721: CompactNumberFormat​() constructor does not comply with spec and format​() method spec for IAEx is not complaint

[Nishit Jain]

Thanks Naoto,

Updated: http://cr.openjdk.java.net/~nishjain/8217254_8217721/webrev.01/

Regards,
Nishit Jain
On 06-03-2019 23:24, naoto.s...@oracle.com wrote:

Hi Nishit,

Just one comment on j.t.CompactNumberFormat.java. At line 425, Null 
check can be done at the top of the method, as a parameter check, so 
that all the unnecessary "if-elseif" can be avoided. Others look good.


Naoto


On 3/6/19 3:56 AM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-8217254 and JDK-8217721

Bug: https://bugs.openjdk.java.net/browse/JDK-8217254
  https://bugs.openjdk.java.net/browse/JDK-8217721

Webrev: http://cr.openjdk.java.net/~nishjain/8217254_8217721/webrev.00/


Issue: The exception thrown by constructor and format() method was 
not compliant with the specification
Fix: Updated the constructor and format method to throw exception as 
per the specification


Regards,
Nishit Jain

[13] RFR 8217254, 8217721: CompactNumberFormat​() constructor does not comply with spec and format​() method spec for IAEx is not complaint

[Nishit Jain]

Hi,

Please review the fix for JDK-8217254 and JDK-8217721

Bug: https://bugs.openjdk.java.net/browse/JDK-8217254
 https://bugs.openjdk.java.net/browse/JDK-8217721

Webrev: http://cr.openjdk.java.net/~nishjain/8217254_8217721/webrev.00/


Issue: The exception thrown by constructor and format() method was not 
compliant with the specification
Fix: Updated the constructor and format method to throw exception as per 
the specification


Regards,
Nishit Jain

[13] RFR 8209175: Handle 'B' character introduced in CLDR 33 JDK update for Burmese (my) locale

[Nishit Jain]

Hi,

Please review the fix for JDK-8209175

Bug: https://bugs.openjdk.java.net/browse/JDK-8209175
Webrev: http://cr.openjdk.java.net/~nishjain/8209175/webrev.03/


Issue: In CLDR 33 update, 'B' character has been added in date time 
patterns for Burmese locale, which is used to represent the day period. 
This character is not supported in DateTimeFormatter or SimpleDateFormat 
APIs, so an exception is thrown that an "unknown character is encountered"


Fix: A workaround is added in which 'B' character appearing in CLDR 
date-time pattern is replaced with 'a' character and hence resolved with 
am/pm strings. This is based on the LDML specification given for 'B' 
character in which it falls back to 'a' character if it is not supported 
by any locale.



Regards,
Nishit Jain

Re: [13] RFR: CONFIG level logging statements printed in CLDRCalendarDataProviderImpl.java even when default log Level is INFO

[Nishit Jain]

Hi Naoto,

Thanks for the explanation. Change looks fine to me.

Regards,
Nishit Jain
On 19-02-2019 22:51, Naoto Sato wrote:

Hi Nishit,

The reason is that "US" is the only required locale in the JDK (cf. 
Locale.getAvailableLocales(). In fact, initially I supplied "001" with 
it, as it means the "world" in CLDR, but it broke some existing tests. 
"001" returns "MONDAY" as the first day of week, whereas "SUNDAY" in 
US. For the compatibility reason, I had to resort to "US". I am not 
sure we want to hardcode "1" in this case without any convincing reason.


Naoto

On 2/19/19 6:37 AM, Nishit Jain wrote:

Hi Naoto,

Why is the default region set to "US" if there is no region specified 
in the locale? is this the default behavior of "first day of week" 
and "minimal days in first week" when a region is missing or the 
default behavior is that it returns "1"? Can't we just return "1" 
instead of setting the region to "US"?


Regards,
Nishit Jain
On 16-02-2019 04:25, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8218960

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8218960/webrev.00/

The CONFIG message was generated because 
CLDRCalendarDataProviderImpl was returning null for locales without 
region. Use "US" as the default region in such a case.


Naoto

Re: [13] RFR: CONFIG level logging statements printed in CLDRCalendarDataProviderImpl.java even when default log Level is INFO

[Nishit Jain]

Hi Naoto,

Why is the default region set to "US" if there is no region specified in 
the locale? is this the default behavior of "first day of week" and 
"minimal days in first week" when a region is missing or the default 
behavior is that it returns "1"? Can't we just return "1" instead of 
setting the region to "US"?


Regards,
Nishit Jain
On 16-02-2019 04:25, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8218960

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8218960/webrev.00/

The CONFIG message was generated because CLDRCalendarDataProviderImpl 
was returning null for locales without region. Use "US" as the default 
region in such a case.


Naoto

Re: RFR(XS):JDK-8219228: java/util/Base64/TestEncodingDecodingLength.java failing on 8GB test machine.

[Nishit Jain]

Hi Arno,

Although I don't know if turning off or no swap is expected for a test 
environment, but tried reproducing the issue locally on a 8Gb Ubuntu 
linux VM with swap turned off, the test case was passing until I started 
some other app (like browser) in parallel, in which case it failed with 
error = "Not enough space". Since it is a memory intensive test, I think 
it is better and safe to increase "os.maxMemory" check


Change looks good to me, I am not an openJDK reviewer though.

Regards,
Nishit Jain
On 18-02-2019 18:19, Zeller, Arno wrote:

Hello!

I found that the test java/util/Base64/TestEncodingDecodingLength.java fails on 
a machine with 8GB memory after JDK-8218265.
The test starts a VM with -Xmx8GB but the VM needs some more memory than only 
the heap and on machines with just 8GB of memory (and no swap configured) the 
test will always fail because the VM cannot get enough native memory.
Therefore I suggest to increase "@requires os.maxMemory" to  >= 10GB to be safe.

Could someone please review this minimal change?

Bug: https://bugs.openjdk.java.net/browse/JDK-8219228

Webrev: http://cr.openjdk.java.net/~azeller/webrevs/8219228/

Thanks and best regards,
Arno

Re: [13] RFR 8217969, 8218265: Base64.Decoder.decode methods ... and java/util/Base64/TestEncodingDecodingLength.java failing

[Nishit Jain]

Thanks Roger, Naoto for reviewingthe changes

Updated the webrev
- Used ByteBuffer.wrap(inputBytes) instead of ByteBuffer.allocate(size)
- Added Decoder.decode(ByteBuffer)method test. (line 58)

http://cr.openjdk.java.net/~nishjain/8217969_8218265/webrev.01/

Regards,
Nishit Jain
On 05-02-2019 00:03, Naoto Sato wrote:

+1

Naoto

On 2/4/19 10:18 AM, Roger Riggs wrote:

Hi Nishit,

Looks fine.

Thanks for fixing the test and improving the implementation.

The test could save one 2G allocation by using 
ByteBuffer.wrap(inputBytes)

instead of ByteBuffer.allocate(size).

Thanks, Roger



On 02/04/2019 10:31 AM, Nishit Jain wrote:

Hi,

Please review the below fix for 8217969 and 8218265

Bug: https://bugs.openjdk.java.net/browse/JDK-8217969
 https://bugs.openjdk.java.net/browse/JDK-8218265

Webrev: http://cr.openjdk.java.net/~nishjain/8217969_8218265/webrev.00/

Issue: Fix for 8210583 changed the Base64.Decoder.decode to throw 
OOME when an integer value overflows, which is an intermediate 
value, not the final value, the final value is always less than 
Integer.MAX_VALUE. Also, the test case written for 8210583, fails on 
certain platforms and throws "OutOfMemoryError: Requested array size 
exceeds VM limit ", due to the array size of length 
Integer.MAX_VALUE - 2.


Fix: Handled the integer overflow by storing the intermediate value 
as "long" and converting the final value as integer. Also, updating 
the test case input array length to Integer.MAX_VALUE - 8, since 
MAX_VALUE - 2 was used only to test the overflow condition in 
Decoder.decode() which is no longer there.


Regards,
Nishit Jain

[13] RFR 8217969, 8218265: Base64.Decoder.decode methods ... and java/util/Base64/TestEncodingDecodingLength.java failing

[Nishit Jain]

Hi,

Please review the below fix for 8217969 and 8218265

Bug: https://bugs.openjdk.java.net/browse/JDK-8217969
 https://bugs.openjdk.java.net/browse/JDK-8218265

Webrev: http://cr.openjdk.java.net/~nishjain/8217969_8218265/webrev.00/

Issue: Fix for 8210583 changed the Base64.Decoder.decode to throw OOME 
when an integer value overflows, which is an intermediate value, not the 
final value, the final value is always less than Integer.MAX_VALUE. 
Also, the test case written for 8210583, fails on certain platforms and 
throws "OutOfMemoryError: Requested array size exceeds VM limit ", due 
to the array size of length Integer.MAX_VALUE - 2.


Fix: Handled the integer overflow by storing the intermediate value as 
"long" and converting the final value as integer. Also, updating the 
test case input array length to Integer.MAX_VALUE - 8, since MAX_VALUE - 
2 was used only to test the overflow condition in Decoder.decode() which 
is no longer there.


Regards,
Nishit Jain

Re: [13] RFR: 8217609: New era placeholder not recognized by java.text.SimpleDateFormat

[Nishit Jain]

Hi Naoto,

Look good to me.

Regards,
Nishit Jain
On 26-01-2019 01:29, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8217609

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8217609/webrev.00/

Although the behavior described in the issue is the expected one 
(i.e., CLDR currently does not provide names for the new era), 
supplementing the name in the CLDR data is desirable from the user's 
point of view.


Naoto

Re: [13] RFR: 8210583: Base64.Encoder incorrectly throws NegativeArraySizeException

[Nishit Jain]

Hi Joe,

Executing it on Mach5 platforms took around 3s to 5s. Below is the link

https://java.se.oracle.com:10065/mdash/jobs/nishjain-jdk_04_09-20190121-0805-19402/results?search=status%3A*

Also executed using JMH on local linux-64 VM (6GB RAM), it took ~2200 ms 
in SingleShotTime mode


Benchmark   Mode  Cnt Score  Error  Units
MyBenchmark.testMethod    ss   50  2200.663 ± 1826.348  ms/op

Regards,
Nishit Jain
On 19-01-2019 06:09, Joe Darcy wrote:
How long does the regression test take to run on machines that have 
enough memory?


Thanks,

-Joe

On 1/18/2019 3:38 PM, naoto.s...@oracle.com wrote:

Looks good, Nishit.

Naoto

On 1/18/19 3:03 AM, Nishit Jain wrote:

Hi Roger, Naoto,

 > The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

When Integer.MAX_VALUE - 8 is used, the decode methods do not throw 
the added OOME, because the computed value does not overflow 
Integer.MAX_VALUE, so to test the added OOME condition need to use 
Integer.MAX_VALUE - 2.


Updated the webrev with rest of the suggestions

http://cr.openjdk.java.net/~nishjain/8210583/webrev.04/

Regards,
Nishit Jain
On 18-01-2019 02:44, Naoto Sato wrote:
I agree with 'withOutputParam' comment. It does seem to require 
some explanation. Same for the newly introduced return value -1.


The test:
  46 // A separate output array is not required, as it is 
not used and
  47 // the exception is thrown beforehand while 
calculating the size

  48 // of the encoded bytes using input array

This seems to make an assumption on the implementation. Test should 
not depend on the internal impl.


Naoto

On 1/17/19 8:14 AM, Roger Riggs wrote:

Hi Nishit,

In the test, there are a couple of RuntimeExceptions with messages 
that start with "As expected"...


That's counter intuitive, that a failure of the test is reported 
with a message saying, its normal!

I would use a message like:  "XXException should have been thrown..."

The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

java/util/Base64.java:

Lines 335-342:  This optimization is not mentioned in the bug 
report and

should be a separate review.


245, 686:  in outLength(), the 2nd parameter would be easier to 
understand

as 'throwOOME', meaning to throw OOM if length is too large.
The 'withOutputParam' only has any meaning in the context of the 
caller.
And even for the private method write the javadoc describing the 
behavior.


It also makes the call sites clearer, the argument will be true to 
throw.


Thanks, Roger


On 01/17/2019 02:07 AM, Nishit Jain wrote:

Hi,

Please review the fix for 8210583

Bug: https://bugs.openjdk.java.net/browse/JDK-8210583
Webrev: http://cr.openjdk.java.net/~nishjain/8210583/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8215633

Issue: Base64.Encoder.encode and Base64.Decoder.decode methods 
incorrectly throw unspecified exception e.g. 
NegativeArraySizeException, when the input byte array size is too 
large such that the calculated output byte size goes beyond the 
max-integer boundary and wraps around.


Fix: Throw an OutOfMemoryError if the output byte array/buffer or 
memory can not be allocated. There is an unrelated change in 
encodeToString(byte[]) where a string instance is created using 
JavaLangAccess.newStringNoRepl(byte[], ISO_8859_1)instead of 
string constructor, to save memory.


Regards,
Nishit Jain

Re: [13] RFR: 8210583: Base64.Encoder incorrectly throws NegativeArraySizeException

[Nishit Jain]

Thanks Roger,

Updated.
http://cr.openjdk.java.net/~nishjain/8210583/webrev.05/

Regards,
Nishit Jain
On 18-01-2019 21:13, Roger Riggs wrote:

Hi Nishit,

Looks good, with a minor fix.

ok, the rationale for MAX_VALUE-2 make sense.

TestEncodingDecodingLength: Line 61 and 68,
  The error message will be more readable with a ": " before the 
methodname is appended.


Thanks, Roger


On 01/18/2019 06:03 AM, Nishit Jain wrote:

Hi Roger, Naoto,

> The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

When Integer.MAX_VALUE - 8 is used, the decode methods do not throw 
the added OOME, because the computed value does not overflow 
Integer.MAX_VALUE, so to test the added OOME condition need to use 
Integer.MAX_VALUE - 2.


Updated the webrev with rest of the suggestions

http://cr.openjdk.java.net/~nishjain/8210583/webrev.04/

Regards,
Nishit Jain
On 18-01-2019 02:44, Naoto Sato wrote:
I agree with 'withOutputParam' comment. It does seem to require some 
explanation. Same for the newly introduced return value -1.


The test:
  46 // A separate output array is not required, as it is 
not used and
  47 // the exception is thrown beforehand while calculating 
the size

  48 // of the encoded bytes using input array

This seems to make an assumption on the implementation. Test should 
not depend on the internal impl.


Naoto

On 1/17/19 8:14 AM, Roger Riggs wrote:

Hi Nishit,

In the test, there are a couple of RuntimeExceptions with messages 
that start with "As expected"...


That's counter intuitive, that a failure of the test is reported 
with a message saying, its normal!

I would use a message like:  "XXException should have been thrown..."

The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

java/util/Base64.java:

Lines 335-342:  This optimization is not mentioned in the bug 
report and

should be a separate review.


245, 686:  in outLength(), the 2nd parameter would be easier to 
understand

as 'throwOOME', meaning to throw OOM if length is too large.
The 'withOutputParam' only has any meaning in the context of the 
caller.
And even for the private method write the javadoc describing the 
behavior.


It also makes the call sites clearer, the argument will be true to 
throw.


Thanks, Roger


On 01/17/2019 02:07 AM, Nishit Jain wrote:

Hi,

Please review the fix for 8210583

Bug: https://bugs.openjdk.java.net/browse/JDK-8210583
Webrev: http://cr.openjdk.java.net/~nishjain/8210583/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8215633

Issue: Base64.Encoder.encode and Base64.Decoder.decode methods 
incorrectly throw unspecified exception e.g. 
NegativeArraySizeException, when the input byte array size is too 
large such that the calculated output byte size goes beyond the 
max-integer boundary and wraps around.


Fix: Throw an OutOfMemoryError if the output byte array/buffer or 
memory can not be allocated. There is an unrelated change in 
encodeToString(byte[]) where a string instance is created using 
JavaLangAccess.newStringNoRepl(byte[], ISO_8859_1)instead of 
string constructor, to save memory.


Regards,
Nishit Jain

Re: [13] RFR: 8210583: Base64.Encoder incorrectly throws NegativeArraySizeException

[Nishit Jain]

Hi Roger, Naoto,

> The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

When Integer.MAX_VALUE - 8 is used, the decode methods do not throw the 
added OOME, because the computed value does not overflow 
Integer.MAX_VALUE, so to test the added OOME condition need to use 
Integer.MAX_VALUE - 2.


Updated the webrev with rest of the suggestions

http://cr.openjdk.java.net/~nishjain/8210583/webrev.04/

Regards,
Nishit Jain
On 18-01-2019 02:44, Naoto Sato wrote:
I agree with 'withOutputParam' comment. It does seem to require some 
explanation. Same for the newly introduced return value -1.


The test:
  46 // A separate output array is not required, as it is not 
used and
  47 // the exception is thrown beforehand while calculating 
the size

  48 // of the encoded bytes using input array

This seems to make an assumption on the implementation. Test should 
not depend on the internal impl.


Naoto

On 1/17/19 8:14 AM, Roger Riggs wrote:

Hi Nishit,

In the test, there are a couple of RuntimeExceptions with messages 
that start with "As expected"...


That's counter intuitive, that a failure of the test is reported with 
a message saying, its normal!

I would use a message like:  "XXException should have been thrown..."

The size of Integer.MAX_VALUE - 2 is implementation specific,
I think more typically max -8 is used to get the biggest allocation.

java/util/Base64.java:

Lines 335-342:  This optimization is not mentioned in the bug report and
should be a separate review.


245, 686:  in outLength(), the 2nd parameter would be easier to 
understand

as 'throwOOME', meaning to throw OOM if length is too large.
The 'withOutputParam' only has any meaning in the context of the caller.
And even for the private method write the javadoc describing the 
behavior.


It also makes the call sites clearer, the argument will be true to 
throw.


Thanks, Roger


On 01/17/2019 02:07 AM, Nishit Jain wrote:

Hi,

Please review the fix for 8210583

Bug: https://bugs.openjdk.java.net/browse/JDK-8210583
Webrev: http://cr.openjdk.java.net/~nishjain/8210583/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8215633

Issue: Base64.Encoder.encode and Base64.Decoder.decode methods 
incorrectly throw unspecified exception e.g. 
NegativeArraySizeException, when the input byte array size is too 
large such that the calculated output byte size goes beyond the 
max-integer boundary and wraps around.


Fix: Throw an OutOfMemoryError if the output byte array/buffer or 
memory can not be allocated. There is an unrelated change in 
encodeToString(byte[]) where a string instance is created using 
JavaLangAccess.newStringNoRepl(byte[], ISO_8859_1)instead of string 
constructor, to save memory.


Regards,
Nishit Jain

Re: [13] RFR: 8216969: ParseException thrown for certain months with russian locale

[Nishit Jain]

Looks Good.

Regards,
Nishit Jain
On 17-01-2019 22:07, Naoto Sato wrote:

Hi Nishit,

Thanks. Updated:

http://cr.openjdk.java.net/~naoto/8216969/webrev.01/

Naoto

On 1/17/19 2:57 AM, Nishit Jain wrote:

Hi Naoto,

Looks good to me. Just a small suggestion.

- To improve readability, can we declare "standalone mask" (0x8000) 
as a static field and use that at all the places?


Regards,
Nishit Jain
On 17-01-2019 05:50, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8216969

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8216969/webrev.00/

For parsing the context sensitive month 'M', the logic was to look 
for the best match for the short name regardless of the styles. 
Changed the parsing code to take the context into account.


Naoto

Re: [13] RFR: 8216969: ParseException thrown for certain months with russian locale

[Nishit Jain]

Hi Naoto,

Looks good to me. Just a small suggestion.

- To improve readability, can we declare "standalone mask" (0x8000) as a 
static field and use that at all the places?


Regards,
Nishit Jain
On 17-01-2019 05:50, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8216969

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8216969/webrev.00/

For parsing the context sensitive month 'M', the logic was to look for 
the best match for the short name regardless of the styles. Changed 
the parsing code to take the context into account.


Naoto

Re: Compact Number Formatting and Fraction Digits

[Nishit Jain]

Hi Gunnar,

Currently there is no way to obtain the below expected behavior (to get 
1K) when min fraction digit is set to non-zero value. I think that is 
not even expected when min fraction digits is set, but considering the 
objective of compact number formatting this could be a good value add to 
introduce an API which if set, truncates trailing fractional zeros while 
formatting output. This may need some thought process on its feasibility.


Regards,
Nishit Jain
On 17-01-2019 14:37, Gunnar Morling wrote:

Hi,

I took a look at the compact number formatting recently added in JDK 12.

There's setMinimumFractionDigits() to control the number of fractional
digits, so that e.g. 1,500 can be formatted as 1.5K. That's great, but
it also will format 1,000 as 1.0K. Is there a way to have fractional
digits but remove trailing zeros, so that 1,500 and 1,000 would be
formatted as 1.5K and 1K, respectively?

Thanks,

--Gunnar

[13] RFR: 8210583: Base64.Encoder incorrectly throws NegativeArraySizeException

[Nishit Jain]

Hi,

Please review the fix for 8210583

Bug: https://bugs.openjdk.java.net/browse/JDK-8210583
Webrev: http://cr.openjdk.java.net/~nishjain/8210583/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8215633

Issue: Base64.Encoder.encode and Base64.Decoder.decode methods 
incorrectly throw unspecified exception e.g. NegativeArraySizeException, 
when the input byte array size is too large such that the calculated 
output byte size goes beyond the max-integer boundary and wraps around.


Fix: Throw an OutOfMemoryError if the output byte array/buffer or memory 
can not be allocated. There is an unrelated change in 
encodeToString(byte[]) where a string instance is created using 
JavaLangAccess.newStringNoRepl(byte[], ISO_8859_1)instead of string 
constructor, to save memory.


Regards,
Nishit Jain

Re: [12] RFR: 8215303: Allowing additional currency code points from later Unicode updates

[Nishit Jain]

Changes looks fine to me.

Regards,
Nishit Jain
On 03-01-2019 22:26, Naoto Sato wrote:

Hello,

Please review the fix to the following issue (and its approved CSR):

https://bugs.openjdk.java.net/browse/JDK-8215303
https://bugs.openjdk.java.net/browse/JDK-8215305

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8215303/webrev.00/

This is a spec only modification that will allow future code point 
additions to the Unicode currency symbols block without any spec 
changes. This is especially important for update releases (such as 8u) 
without releasing a Maintenance Release.


Naoto

[12] RFR JDK-8214935: Upgrade IANA LSR data

[Nishit Jain]

Hi,

Please review the fix for JDK-8214935. The changes made through 
JDK-8213294 upgraded the LSR data to 2018-10-31, but there is a recent 
change seen in the LSR data with version 2018-11-30.
The JDK-8214935 change is to stick to the latest available version for 
JDK12.


RFE: https://bugs.openjdk.java.net/browse/JDK-8214935
Webrev: http://cr.openjdk.java.net/~nishjain/8214935/webrev.00/

Regards,
Nishit Jain

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Thanks Roger,

Updated Webrev: 
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.06/


Regards,
Nishit Jain
On 04-12-2018 00:58, Roger Riggs wrote:

Hi Nishit,

Thanks for the updates and cleanup.

CompactNumberFormat.java:

- 827: To locate a single character use:
    if (pattern.indexOf(QUOTE) < 0) { ... }

OK.


- 1488:  Since infinite returns do not depend on any of the code after 
line 1454,

   the 1488- 1494 could be moved to 1454. (It is correct where it is).
The computeParseMultiplier decides whether parse string is positive or 
negative based on the prefix and suffix, so the status[STATUS_POSITIVE] 
can be also be used to return positive or negative infinity.


Other minor changes in parse():

- Taken out "int numPosition" (line 1444 in webrev.05) and used earlier 
defined variable "position" instead, as "position" previous value is not 
used after that statement.
- Moved the variable "Number multiplier;" (line 1447 in webrev.05) to 
the place where it is assigned the value.

- Moved "Number cnfResult;" (line 1496 in webrev.05) inside else block.



 - in API design, I would have put the position argument immediately 
after text since the position
   is closely related to the text argument (in matchAffix and 
matchPrefixAndSuffix methods).

   Its probably not worth the change in these private methods.
Yes, it is better to move it next to "text". Updated both "matchAffix" 
and "matchPrefixAndSuffix" methods.


Regards,
Nishit Jain


comments below...

On 12/03/2018 07:22 AM, Nishit Jain wrote:

Thanks Roger,

Updated the webrev based on thebelow suggested changes and some clean 
up.


http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/

On 01-12-2018 01:03, Roger Riggs wrote:

Hi Nishit,

Some comments, a couple of possible bugs and several performance 
related issues
could be deferred. Both formatting and parsing seem to be quite 
heavyweight

due to looping and combinatorics.


CompactNumberFormat.java


661, 727, 1507: Is there a reason to convert the divisor to a string 
and then re-parse it to create a new BigDecimal?
    (IntelliJ observes that divide is called without a rounding 
argument.)
    Given that the divisors are all powers of 10 and the digit 
list is a base 10 set of digits
    it seems more efficient to just move the decimal point then 
to do the math.
    BTW, the divisor.toString() is preferred over concat with 
"".  (looks like a hack).


    It would be more efficient to write two methods that would 
pass the Number
    and return a BigInteger or BigDecimal by dispatching on the 
concrete type and

    using the appropriate constructor.
Changed concatenation with toString() and added a rounding parameter, 
but not getting the benefit of having two methods and returning 
respective concrete type using constructors.

I didn't get the point of having two methods. Can you please elaborate?
The would the same function but different return types (BigInteger vs 
BigDecimal).

The code is ok as is.


781:  @param prefix - the parameter name is suffix

804: move the int start = inside the if.

826:  expandAffix can be more efficient if tests the pattern for the 
presence of QUOTE
    and returns the pattern if the QUOTE is not present. That 
would be the most common case.


914: Reduce the number of compares by reordering to:
    if number > currentValue; multiply and continue
    if number < currentValue break;
    else ==; assign matched index and break;

    In the normal case, there will be only one compare in the 
loop until it is to exit.


1109:  IntelliJ observes that within the case QUOTE, the if (ch == 
QUOTE) is always true

  so the if is redundant.

OK.


1205:  It looks odd to prepend two characters '- to the prefix. Is 
the single quote correct?

  Where's the closing quote if so.
It is to specify that the minus sign is a special character, which 
needs to be replaced with its localized equivalent at the time of 
formatting.


Internally, there is a difference between a "minus sign prefix with a 
single quote" and a "minus sign" (it depends on how user specify the 
pattern), because the later one is considered as a literal and should 
be used exactly same in the formatted output, but the one prefixed 
with a single quote is replaced with its localized symbol using 
DecimalFormatSymbols.getMinusSign().

thanks for the explanation.



1394: matchedPosPrefix.length() is compared to negativePrefix.length().
  That's an unusual mixing of positive and negative! Please 
re-check.

Yes, it was a mistake.


1363:  Can there be an early exit from this loop if one of the 
prefixes is identified?
  The pattern of comparing for a prefix/suffix and the length 
might warrant
  creating a private method to reduce the repetitive parts of 
the code.
I had thought about i

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Thanks Roger,

Updated the webrev based on thebelow suggested changes and some clean up.

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/

On 01-12-2018 01:03, Roger Riggs wrote:

Hi Nishit,

Some comments, a couple of possible bugs and several  performance 
related issues
could be deferred. Both formatting and parsing seem to be quite 
heavyweight

due to looping and combinatorics.


CompactNumberFormat.java


661, 727, 1507: Is there a reason to convert the divisor to a string 
and then re-parse it to create a new BigDecimal?
    (IntelliJ observes that divide is called without a rounding 
argument.)
    Given that the divisors are all powers of 10 and the digit 
list is a base 10 set of digits
    it seems more efficient to just move the decimal point then to 
do the math.
    BTW, the divisor.toString() is preferred over concat with "".  
(looks like a hack).


    It would be more efficient to write two methods that would 
pass the Number
    and return a BigInteger or BigDecimal by dispatching on the 
concrete type and

    using the appropriate constructor.
Changed concatenation with toString() and added a rounding parameter, 
but not getting the benefit of having two methods and returning 
respective concrete type using constructors.

I didn't get the point of having two methods. Can you please elaborate?


781:  @param prefix - the parameter name is suffix

804: move the int start = inside the if.

826:  expandAffix can be more efficient if tests the pattern for the 
presence of QUOTE
    and returns the pattern if the QUOTE is not present.  That 
would be the most common case.


914: Reduce the number of compares by reordering to:
    if number > currentValue; multiply and continue
    if number < currentValue break;
    else ==; assign matched index and break;

    In the normal case, there will be only one compare in the loop 
until it is to exit.


1109:  IntelliJ observes that within the case QUOTE, the if (ch == 
QUOTE) is always true

  so the if is redundant.

OK.


1205:  It looks odd to prepend two characters '- to the prefix. Is the 
single quote correct?

  Where's the closing quote if so.
It is to specify that the minus sign is a special character, which needs 
to be replaced with its localized equivalent at the time of formatting.


Internally, there is a difference between a "minus sign prefix with a 
single quote" and a "minus sign" (it depends on how user specify the 
pattern), because the later one is considered as a literal and should be 
used exactly same in the formatted output, but the one prefixed with a 
single quote is replaced with its localized symbol using 
DecimalFormatSymbols.getMinusSign().



1394: matchedPosPrefix.length() is compared to negativePrefix.length().
  That's an unusual mixing of positive and negative! Please re-check.

Yes, it was a mistake.


1363:  Can there be an early exit from this loop if one of the 
prefixes is identified?
  The pattern of comparing for a prefix/suffix and the length 
might warrant
  creating a private method to reduce the repetitive parts of the 
code.
I had thought about it, but it was difficult unless the whole list of 
affixes are traversed, because there is always a chance to get longer 
affix later in the list. I thought to sort the lists based on the length 
and do the match, but in that case indexes get disordered across lists 
(divisor, prefix, suffix, compact patterns), and computation will become 
more complicated.
Updated the code by moving the repetitive parts in the loop to private 
methods.


1593: extra space between "- ("; should be the same style as 1591

1627, 1363: Is an early exit from this loop possible?
   or a quick comparison to avoid the regionMatches.
   Do the regionMatches *only if* the prefix/suffix is longer than 
the suffix already compared?

Yes, I think this can be done. Updated.


1721:  IntelliJ observes that if (gotNeg) is redundant since 1708 
rules out them being both true or both false.

Updated

Regards,
Nishit Jain


Thanks, Roger



On 11/28/18 3:46 AM, Nishit Jain wrote:

Thanks Naoto,

Updated.

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.04/

[12] RFR 8213294: Upgrade IANA LSR data

[Nishit Jain]

Hi,

Please review the fix for 8213294, which upgrades the IANA LSR data to 
the latest available version.


Bug: https://bugs.openjdk.java.net/browse/JDK-8213294
Webrev: http://cr.openjdk.java.net/~nishjain/8213294/webrev.00


Regards,
Nishit Jain

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Thanks Naoto,

Updated.

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.04/

Regards,
Nishit Jain
On 27-11-2018 20:55, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/26/18 11:11 PM, Nishit Jain wrote:

Hi Naoto,

On 26-11-2018 21:01, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/26/18 12:41 AM, Nishit Jain wrote:

Hi Naoto,

To add to my previous mail comment, the DecimalFormat spec also 
says that


/*"DecimalFormat can be instructed to format and parse scientific 
notation only via a pattern; there is currently no factory method 
that creates a scientific notation format. In a pattern, the 
exponent character immediately followed by one or more digit 
characters indicates scientific notation. "


*/That is, exponent formatting and parsing is instructed only via a 
scientific notation pattern and I think should not be there with 
*general number* formatting.


I am not sure the quoted sentence should be interpreted that way. My 
understanding is that the section means there is no public 
NumberFormat.getScientificInstance() method (cf. line 601 at 
NumberFormat.java), so that users will have to use 'E' in their 
pattern string.


Anyway, my point is that if you prefer to treat the scientific 
notation differently between DecimalFormat and CompactDecimalFormat, 
then it will need to be clarified in the spec. Personally I agree 
that it is not practical to interpret E in the CNF.
OK. If it is better to specify the parsing behavior w.r.t. the 
exponential numbers, I have added a statement in the parse() API.


*/"CompactNumberFormat parse does not allow parsing exponential 
number strings. For example, parsing a string "1.05E4K" in US locale 
breaks at character 'E' and returns 1.05."/*


That's better. I'd replace "exponential number strings" with 
"scientific notations." You may want to check the final wording with 
English natives.




Updated the webrev
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.03/

Will also update the CSR and refinalize it.


Thanks.

Naoto



Regards,
Nishit Jain


Naoto



Updated webrev based on the other comments

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/

 > Some more comments (all in CompactNumberFormat.java)

 > line 807: expandAffix() seems to treat localizable special 
pattern characters, but currently the implementation only cares for 
the minus sign. Should other localizable pattern chars be taken 
care of, such as percent sign?
- Other special characters like '%' percent sign are not allowed as 
per CNF compact pattern spec


 > line 869, 888: Define what -1 means as a ret value.
- OK.

 > line 897: iterMultiplier be better all capitalized as it is a 
constant. And it could be statically defined in the class to be 
shared with other locations that use "10" for arithmetic operation.

- OK, made it static final and renamed it as RANGE_MULTIPLIER

 > line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue 
like JDK-8211161, which we know is not an issue.


Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:

Hi Naoto,

> I think DecimalFormat and CNF should behave the same, ie. 'E' 
should be treated as the exponent without a quote.


Personally I don't think that the exponential parsing should be 
supported by CompactNumberFormat, because the objective of compact 
numbers is to represent numbers in short form. So, parsing of 
number format like "1.05E4K" should not be expected from 
CompactNumberFormat, I am even doubtful that such forms 
("1.05E4K") are used anywhere where exponential and compact form 
are together used. If formatting and parsing of exponential 
numbers are needed it should be done by DecimalFormat scientific 
instance *not *with the general number instance.So, I don't think 
that we should allow parsing of exponential numbers.Comments welcome.


Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/21/18 12:53 AM, Nishit Jain wrote:

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the 
resource bundles


Good.

- refactored DecimalFormat.subparse() to be used by the 
CNF.parse(), to reduce code duplication.


I presume CNF is calling package-private methods in DF to share 
the same code. Some comments noting the sharing would be helpful.



- Also updated it with other changes as suggested in the comments


Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should 
CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this 
aspect?


I think DecimalFormat and CNF should behave the same, ie. 'E' 
sho

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi Naoto,

On 26-11-2018 21:01, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/26/18 12:41 AM, Nishit Jain wrote:

Hi Naoto,

To add to my previous mail comment, the DecimalFormat spec also says 
that


/*"DecimalFormat can be instructed to format and parse scientific 
notation only via a pattern; there is currently no factory method 
that creates a scientific notation format. In a pattern, the exponent 
character immediately followed by one or more digit characters 
indicates scientific notation. "


*/That is, exponent formatting and parsing is instructed only via a 
scientific notation pattern and I think should not be there with 
*general number* formatting.


I am not sure the quoted sentence should be interpreted that way. My 
understanding is that the section means there is no public 
NumberFormat.getScientificInstance() method (cf. line 601 at 
NumberFormat.java), so that users will have to use 'E' in their 
pattern string.


Anyway, my point is that if you prefer to treat the scientific 
notation differently between DecimalFormat and CompactDecimalFormat, 
then it will need to be clarified in the spec. Personally I agree that 
it is not practical to interpret E in the CNF.
OK. If it is better to specify the parsing behavior w.r.t. the 
exponential numbers, I have added a statement in the parse() API.


*/"CompactNumberFormat parse does not allow parsing exponential number 
strings. For example, parsing a string "1.05E4K" in US locale breaks at 
character 'E' and returns 1.05."/*


Updated the webrev
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.03/

Will also update the CSR and refinalize it.

Regards,
Nishit Jain


Naoto



Updated webrev based on the other comments

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/

 > Some more comments (all in CompactNumberFormat.java)

 > line 807: expandAffix() seems to treat localizable special pattern 
characters, but currently the implementation only cares for the minus 
sign. Should other localizable pattern chars be taken care of, such 
as percent sign?
- Other special characters like '%' percent sign are not allowed as 
per CNF compact pattern spec


 > line 869, 888: Define what -1 means as a ret value.
- OK.

 > line 897: iterMultiplier be better all capitalized as it is a 
constant. And it could be statically defined in the class to be 
shared with other locations that use "10" for arithmetic operation.

- OK, made it static final and renamed it as RANGE_MULTIPLIER

 > line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like 
JDK-8211161, which we know is not an issue.


Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:

Hi Naoto,

> I think DecimalFormat and CNF should behave the same, ie. 'E' 
should be treated as the exponent without a quote.


Personally I don't think that the exponential parsing should be 
supported by CompactNumberFormat, because the objective of compact 
numbers is to represent numbers in short form. So, parsing of number 
format like "1.05E4K" should not be expected from 
CompactNumberFormat, I am even doubtful that such forms ("1.05E4K") 
are used anywhere where exponential and compact form are together 
used. If formatting and parsing of exponential numbers are needed it 
should be done by DecimalFormat scientific instance *not *with the 
general number instance.So, I don't think that we should allow 
parsing of exponential numbers.Comments welcome.


Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/21/18 12:53 AM, Nishit Jain wrote:

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the 
resource bundles


Good.

- refactored DecimalFormat.subparse() to be used by the 
CNF.parse(), to reduce code duplication.


I presume CNF is calling package-private methods in DF to share the 
same code. Some comments noting the sharing would be helpful.



- Also updated it with other changes as suggested in the comments


Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should 
CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this 
aspect?


I think DecimalFormat and CNF should behave the same, ie. 'E' 
should be treated as the exponent without a quote.


Some more comments (all in CompactNumberFormat.java)

line 807: expandAffix() seems to treat localizable special pattern 
characters, but currently the implementation only cares for the 
minus sign. Should other localizable pattern chars be taken care 
of, such as percent sign?


line 869, 888: Define what -1 means as a ret value.

line 897: iterMultiplier be b

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi Naoto,

To add to my previous mail comment, the DecimalFormat spec also says that

/*"DecimalFormat can be instructed to format and parse scientific 
notation only via a pattern; there is currently no factory method that 
creates a scientific notation format. In a pattern, the exponent 
character immediately followed by one or more digit characters indicates 
scientific notation. "


*/That is, exponent formatting and parsing is instructed only via a 
scientific notation pattern and I think should not be there with 
*general number* formatting.


Updated webrev based on the other comments

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/

> Some more comments (all in CompactNumberFormat.java)

> line 807: expandAffix() seems to treat localizable special pattern 
characters, but currently the implementation only cares for the minus 
sign. Should other localizable pattern chars be taken care of, such as 
percent sign?
- Other special characters like '%' percent sign are not allowed as per 
CNF compact pattern spec


> line 869, 888: Define what -1 means as a ret value.
- OK.

> line 897: iterMultiplier be better all capitalized as it is a 
constant. And it could be statically defined in the class to be shared 
with other locations that use "10" for arithmetic operation.

- OK, made it static final and renamed it as RANGE_MULTIPLIER

> line 1531: Any possibility this could lead to divide-by-zero?
- None which I am aware of, unless you are pointing to the issue like 
JDK-8211161, which we know is not an issue.


Regards,
Nishit Jain
On 23-11-2018 15:55, Nishit Jain wrote:

Hi Naoto,

> I think DecimalFormat and CNF should behave the same, ie. 'E' should 
be treated as the exponent without a quote.


Personally I don't think that the exponential parsing should be 
supported by CompactNumberFormat, because the objective of compact 
numbers is to represent numbers in short form. So, parsing of number 
format like "1.05E4K" should not be expected from CompactNumberFormat, 
I am even doubtful that such forms ("1.05E4K") are used anywhere where 
exponential and compact form are together used. If formatting and 
parsing of exponential numbers are needed it should be done by 
DecimalFormat scientific instance *not *with the general number 
instance.So, I don't think that we should allow parsing of exponential 
numbers.Comments welcome.


Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/21/18 12:53 AM, Nishit Jain wrote:

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the 
resource bundles


Good.

- refactored DecimalFormat.subparse() to be used by the CNF.parse(), 
to reduce code duplication.


I presume CNF is calling package-private methods in DF to share the 
same code. Some comments noting the sharing would be helpful.



- Also updated it with other changes as suggested in the comments


Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?

I think DecimalFormat and CNF should behave the same, ie. 'E' should 
be treated as the exponent without a quote.


Some more comments (all in CompactNumberFormat.java)

line 807: expandAffix() seems to treat localizable special pattern 
characters, but currently the implementation only cares for the minus 
sign. Should other localizable pattern chars be taken care of, such 
as percent sign?


line 869, 888: Define what -1 means as a ret value.

line 897: iterMultiplier be better all capitalized as it is a 
constant. And it could be statically defined in the class to be 
shared with other locations that use "10" for arithmetic operation.


line 1531: Any possibility this could lead to divide-by-zero?

Naoto



Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/18/18 10:29 PM, Nishit Jain wrote:

Hi Naoto,

Please check my comments inline.

On 17-11-2018 04:52, naoto.s...@oracle.com wrote:

Hi Nishit,

Here are my comments:

- CLDRConverter: As the compact pattern no more employs 
List, can we eliminate stringListEntry/Element, and use 
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact 
pattern, so at the time of parsing the CLDR xmls using SAX parser, 
it becomes difficult to identify the size of array when the parent 
element of compact pattern is encountered, so I think it is better 
to keep the List while extracting the resources.


OK. However I'd not keep the List format on generating the 
resource bundle, as there is no reason to introduce yet another 
bundle format other than the existing array of String

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi Roger,

Please find my comments belowand check the updated webrev.

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.02/

Regards,
Nishit Jain
On 22-11-2018 00:04, Roger Riggs wrote:

Hi Nishit,

Comments on the tests:

- The tests looks to be quite complete.

- Have the locale specific data been independently verified?
   Or are they just assumed to be correct based on using CNF to 
generate the formatted strings?
I have manually verified the data against the compact patterns in the 
resource bundles to the best I can. In some cases, like testing CNF 
rounding behavior "TestCNFRounding", the data is also verified against 
the output produced by DecimalFormat.


- Is there any overlap between the format and parse patterns that can 
be removed;
   using the same dataprovider for both format and parse (and an extra 
provider for unique cases).
Yes, there were patterns overlap in CompactPatternsValidity (now renamed 
as TestCompactPatternsValidity), patterns are taken out and shared 
between format and parse.


- Using TestNG consistently would improve the test suite.
OK. Updated EqualityCheck and SerializationTest (now named as 
TestEquality and TestSerialization) to use TestNG.


- In comments, Capitalize the first word

- The names of the test files should be more consistent, some include 
Test at the beginning,
   some at the end and some not at all.  The utility classes 
(CompactFormatAndParse) name

   doesn't make it clear it is not a test itself.

Updated.


Serialization Test: should be comparing the fields of the Format 
instances,

not only that it formats a value the same.
It also compares the equality (if (!fmt.equals(obj)))  so fields of the 
instances are also matched.


To setup for future revisions, several serialized CNF instances should 
be hardcoded
in the test and deserialized to be checked against the current CNF 
instances.
Added TestDeserializeCNF.java which deserializes the hardcoded instances 
in cnf1.ser.txt and cnf2.ser.txt. In the comments, also added the API 
used to create the hexdump of the serializable instance, please check if 
that is the correct way.


Using testng dataproviders would show a more regular structure.
I do not find the use of data provider to be useful here, as we just 
have some instances which are serialized and deserialized with no 
specific data to test.


CompactFormatAndParse.java:
 - The method don't need "public" since they are used only in the test.
 - unused import of BigInteger

OK


EqualityCheck:
 - Its good form to always have an @run line, even if for default 
behavior.

Moved it to use TestNG and added corresponding @run line


 - The CNF.equals method includes both symbols and decimal pattern;
   are there tests for those being the different?

Thanks. Added.


CompactPatternsValidity.java:
 -60:  Indentation of continued data array values would make it more 
readable.

OK


 - Is there any overlap between the format and parse patterns that can 
be removed?
   Using the same dataprovider for both format and parse (and an extra 
provider for unique cases).
Yes, modified CompactPatternsValidity.java, as mentioned in the above 
comment


CNFRoundingTest.java:
 - Can the Rounding mode test methods be consolidated and pass in the 
desired rounding mode.

  It would save on some boilerplate.

Yes, updated.

Regards,
Nishit Jain


Thanks, Roger


On 11/21/2018 03:53 AM, Nishit Jain wrote:

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the resource 
bundles
- refactored DecimalFormat.subparse() to be used by the CNF.parse(), 
to reduce code duplication.

- Also updated it with other changes as suggested in the comments

Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/18/18 10:29 PM, Nishit Jain wrote:

Hi Naoto,

Please check my comments inline.

On 17-11-2018 04:52, naoto.s...@oracle.com wrote:

Hi Nishit,

Here are my comments:

- CLDRConverter: As the compact pattern no more employs 
List, can we eliminate stringListEntry/Element, and use 
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact 
pattern, so at the time of parsing the CLDR xmls using SAX parser, 
it becomes difficult to identify the size of array when the parent 
element of compact pattern is encountered, so I think it is better 
to keep the List while extracting the resources.


OK. However I'd not keep the List format on generating the 
resource bundle, as there is no reason to introduce yet another 
bundle format other than the existing array of String.




- CompactNumberFormat.java

Multiple locations: Use StringBuilder instead of StringBuffer.

OK


line 268: The link points to 
NumberFormat.getNumberInstance(Locale) instead of DecimalFormat

OK. Changed it at line 165 also.


line 855: no need to do toString(). length() can detect

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi Naoto,

> I think DecimalFormat and CNF should behave the same, ie. 'E' should 
be treated as the exponent without a quote.


Personally I don't think that the exponential parsing should be 
supported by CompactNumberFormat, because the objective of compact 
numbers is to represent numbers in short form. So, parsing of number 
format like "1.05E4K" should not be expected from CompactNumberFormat, I 
am even doubtful that such forms ("1.05E4K") are used anywhere where 
exponential and compact form are together used. If formatting and 
parsing of exponential numbers are needed it should be done by 
DecimalFormat scientific instance *not *with the general number 
instance.So, I don't think that we should allow parsing of exponential 
numbers.Comments welcome.


Regards,
Nishit Jain
On 22-11-2018 02:02, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/21/18 12:53 AM, Nishit Jain wrote:

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the resource 
bundles


Good.

- refactored DecimalFormat.subparse() to be used by the CNF.parse(), 
to reduce code duplication.


I presume CNF is calling package-private methods in DF to share the 
same code. Some comments noting the sharing would be helpful.



- Also updated it with other changes as suggested in the comments


Sorry I missed your question the last time:
>>> Do you think this is an issue with DecimalFormat.parse() and CNF
>>> should avoid parsing exponential numbers? Or, should CNF.parse() be
>>> modified to be consistent with DecimalFormat.parse() in this aspect?

I think DecimalFormat and CNF should behave the same, ie. 'E' should 
be treated as the exponent without a quote.


Some more comments (all in CompactNumberFormat.java)

line 807: expandAffix() seems to treat localizable special pattern 
characters, but currently the implementation only cares for the minus 
sign. Should other localizable pattern chars be taken care of, such as 
percent sign?


line 869, 888: Define what -1 means as a ret value.

line 897: iterMultiplier be better all capitalized as it is a 
constant. And it could be statically defined in the class to be shared 
with other locations that use "10" for arithmetic operation.


line 1531: Any possibility this could lead to divide-by-zero?

Naoto



Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/18/18 10:29 PM, Nishit Jain wrote:

Hi Naoto,

Please check my comments inline.

On 17-11-2018 04:52, naoto.s...@oracle.com wrote:

Hi Nishit,

Here are my comments:

- CLDRConverter: As the compact pattern no more employs 
List, can we eliminate stringListEntry/Element, and use 
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact 
pattern, so at the time of parsing the CLDR xmls using SAX parser, 
it becomes difficult to identify the size of array when the parent 
element of compact pattern is encountered, so I think it is better 
to keep the List while extracting the resources.


OK. However I'd not keep the List format on generating the 
resource bundle, as there is no reason to introduce yet another 
bundle format other than the existing array of String.




- CompactNumberFormat.java

Multiple locations: Use StringBuilder instead of StringBuffer.

OK


line 268: The link points to 
NumberFormat.getNumberInstance(Locale) instead of DecimalFormat

OK. Changed it at line 165 also.


line 855: no need to do toString(). length() can detect whether 
it's empty or not.


line 884: "Overloaded method" reads odd here. I'd prefer 
specializing in the "given number" into either long or biginteger.

OK


line 1500: subparseNumber() pretty much shares the same code with 
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly 
is handled, DecimalFormat.parse()/subparse() behaviour is 
unpredictable with parseIntegeronly = true when multipliers are 
involved (Please see JDK-8199223).


Also, I had thought that the CNF.parse()/subparseNumber() should 
*not *parse the exponential notation e.g. while parsing "1.05E4K" 
the parsing should break at 'E' and returns 1.05, because 'E' 
should be considered as unparseable character for general number 
format pattern or compact number pattern, but this is not the case 
with DecimalFormat.parse(). The below DecimalFormat general number 
format instance


NumberFormat nf =  NumberFormat.getNumberInstance();
nf.parse("1.05E4")

Successfully parse the string and returns 10500. The same behaviour 
is there with other DecimalFormat instances also e.g. currency 
instance.


Do you think this is an issue with DecimalFormat.parse() and CNF 
should avoid parsing exponential numbers? Or, should CNF.parse() be 
modified to be consistent with DecimalFormat.pa

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi Naoto,

Updated the webrev based on suggestions

http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.01/

Changes made:
- Replaced List with String[] to be added to the the resource 
bundles
- refactored DecimalFormat.subparse() to be used by the CNF.parse(), to 
reduce code duplication.

- Also updated it with other changes as suggested in the comments

Regards,
Nishit Jain
On 20-11-2018 00:33, naoto.s...@oracle.com wrote:

Hi Nishit,

On 11/18/18 10:29 PM, Nishit Jain wrote:

Hi Naoto,

Please check my comments inline.

On 17-11-2018 04:52, naoto.s...@oracle.com wrote:

Hi Nishit,

Here are my comments:

- CLDRConverter: As the compact pattern no more employs 
List, can we eliminate stringListEntry/Element, and use 
Array equivalent instead?
Since the CNF design does not put any limit on the size of compact 
pattern, so at the time of parsing the CLDR xmls using SAX parser, it 
becomes difficult to identify the size of array when the parent 
element of compact pattern is encountered, so I think it is better to 
keep the List while extracting the resources.


OK. However I'd not keep the List format on generating the 
resource bundle, as there is no reason to introduce yet another bundle 
format other than the existing array of String.




- CompactNumberFormat.java

Multiple locations: Use StringBuilder instead of StringBuffer.

OK


line 268: The link points to NumberFormat.getNumberInstance(Locale) 
instead of DecimalFormat

OK. Changed it at line 165 also.


line 855: no need to do toString(). length() can detect whether it's 
empty or not.


line 884: "Overloaded method" reads odd here. I'd prefer 
specializing in the "given number" into either long or biginteger.

OK


line 1500: subparseNumber() pretty much shares the same code with 
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly 
is handled, DecimalFormat.parse()/subparse() behaviour is 
unpredictable with parseIntegeronly = true when multipliers are 
involved (Please see JDK-8199223).


Also, I had thought that the CNF.parse()/subparseNumber() should *not 
*parse the exponential notation e.g. while parsing "1.05E4K" the 
parsing should break at 'E' and returns 1.05, because 'E' should be 
considered as unparseable character for general number format pattern 
or compact number pattern, but this is not the case with 
DecimalFormat.parse(). The below DecimalFormat general number format 
instance


NumberFormat nf =  NumberFormat.getNumberInstance();
nf.parse("1.05E4")

Successfully parse the string and returns 10500. The same behaviour 
is there with other DecimalFormat instances also e.g. currency instance.


Do you think this is an issue with DecimalFormat.parse() and CNF 
should avoid parsing exponential numbers? Or, should CNF.parse() be 
modified to be consistent with DecimalFormat.parse() in this aspect?


No, I understand there are differences. But I see a lot of duplicated 
piece of code which I would like to eliminate.






line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls 
super. No need to override them.
Since setters are overridden, I think that it is better to override 
getters also (even if they are just calling super and have same 
javadoc) to keep them at same level. But, if you see no point in 
keeping them in CNF, I will remove them. Does that need CSR change?


I don't see any point for override. I don't think there needs a CSR, 
but better ask Joe about it.




line 2231: You need to test the type before cast. Otherwise 
ClassCastException may be thrown.
The type is checked in the superclass equals method getClass() != 
obj.getClass(), so I think there is no need to check the type here.


OK.

Naoto



Regards,
Nishit Jain


Naoto

On 11/16/18 9:54 AM, Nishit Jain wrote:

Hi,

Please review this non trivial feature addition to NumberFormat API.

The existing NumberFormat API provides locale based support for 
formatting and parsing numbers which includes formatting decimal, 
percent, currency etc, but the support for formatting a number into 
a human readable or compact form is missing. This RFE adds that 
feature to format a decimal number in a compact format (e.g. 1000 
-> 1K, 100 -> 1M in en_US locale) , which is useful for the 
environment where display space is limited, so that the formatted 
string can be displayed in that limited space. It is defined by 
LDML's specification for Compact Number Formats.


http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats 




RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
Webrev: 
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/

CSR: https://bugs.openjdk.java.net/browse/JDK-8188147

Request to please help review the the change.

Regards,
Nishit Jain

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi Stephen,

Thanks for the comments.

The NumberFormat.Style class just defines two styles and currently it is 
only used by CompactNumberFormat, making it a top level class would 
require it to be more general purpose and increases the scope. In which 
case evolving it at a later point of time would bea risk. The current 
inner NumberFormat.Style has the scope narrow to NF and poses less risk.


Regarding providing control of the compact patterns or text, a user can 
use the CNF constructor and define its own compact patterns/text for 
that instance, compact NumberFormatProvider can also be used to to get 
the customized CNF instance.


Regards,
Nishit Jain

On 19-11-2018 19:42, Stephen Colebourne wrote:

I'm not a big fan of having a class named `Style` as it is commonly used in
business logic. (Yes, its an inner class, but I still think the potential
for annoyance is high). java.time.* has `TextStyle`, but I don't think it
can be reused here. Maybe the class should be honest and called
NumberFormatStyle (as a top level class).

More generally, the API does not allow the caller to take control of the
text, for example to use "mil" as a suffix for million. eg Think of this
method in java.time.* -
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/format/DateTimeFormatterBuilder.html#appendText(java.time.temporal.TemporalField,java.util.Map)


Stephen


On Fri, 16 Nov 2018 at 17:56, Nishit Jain  wrote:


Hi,

Please review this non trivial feature addition to NumberFormat API.

The existing NumberFormat API provides locale based support for
formatting and parsing numbers which includes formatting decimal,
percent, currency etc, but the support for formatting a number into a
human readable or compact form is missing. This RFE adds that feature to
format a decimal number in a compact format (e.g. 1000 -> 1K, 100 ->
1M in en_US locale) , which is useful for the environment where display
space is limited, so that the formatted string can be displayed in that
limited space. It is defined by LDML's specification for Compact Number
Formats.

http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats


RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8188147

Request to please help review the the change.

Regards,
Nishit Jain

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi Naoto,

Please check my comments inline.

On 17-11-2018 04:52, naoto.s...@oracle.com wrote:

Hi Nishit,

Here are my comments:

- CLDRConverter: As the compact pattern no more employs List, 
can we eliminate stringListEntry/Element, and use Array equivalent 
instead?
Since the CNF design does not put any limit on the size of compact 
pattern, so at the time of parsing the CLDR xmls using SAX parser, it 
becomes difficult to identify the size of array when the parent element 
of compact pattern is encountered, so I think it is better to keep the 
List while extracting the resources.


- CompactNumberFormat.java

Multiple locations: Use StringBuilder instead of StringBuffer.

OK


line 268: The link points to NumberFormat.getNumberInstance(Locale) 
instead of DecimalFormat

OK. Changed it at line 165 also.


line 855: no need to do toString(). length() can detect whether it's 
empty or not.


line 884: "Overloaded method" reads odd here. I'd prefer specializing 
in the "given number" into either long or biginteger.

OK


line 1500: subparseNumber() pretty much shares the same code with 
DecimalFormat.subparse(). can they be merged?
The existing CNF.subParseNumber differs in the way parseIntegerOnly is 
handled, DecimalFormat.parse()/subparse() behaviour is unpredictable 
with parseIntegeronly = true when multipliers are involved (Please see 
JDK-8199223).


Also, I had thought that the CNF.parse()/subparseNumber() should *not 
*parse the exponential notation e.g. while parsing "1.05E4K" the parsing 
should break at 'E' and returns 1.05, because 'E' should be considered 
as unparseable character for general number format pattern or compact 
number pattern, but this is not the case with DecimalFormat.parse(). The 
below DecimalFormat general number format instance


NumberFormat nf =  NumberFormat.getNumberInstance();
nf.parse("1.05E4")

Successfully parse the string and returns 10500. The same behaviour is 
there with other DecimalFormat instances also e.g. currency instance.


Do you think this is an issue with DecimalFormat.parse() and CNF should 
avoid parsing exponential numbers? Or, should CNF.parse() be modified to 
be consistent with DecimalFormat.parse() in this aspect?




line 1913-1923, 1950-1960, 1987-1997, 2024-2034: It simply calls 
super. No need to override them.
Since setters are overridden, I think that it is better to override 
getters also (even if they are just calling super and have same javadoc) 
to keep them at same level. But, if you see no point in keeping them in 
CNF, I will remove them. Does that need CSR change?


line 2231: You need to test the type before cast. Otherwise 
ClassCastException may be thrown.
The type is checked in the superclass equals method getClass() != 
obj.getClass(), so I think there is no need to check the type here.


Regards,
Nishit Jain


Naoto

On 11/16/18 9:54 AM, Nishit Jain wrote:

Hi,

Please review this non trivial feature addition to NumberFormat API.

The existing NumberFormat API provides locale based support for 
formatting and parsing numbers which includes formatting decimal, 
percent, currency etc, but the support for formatting a number into a 
human readable or compact form is missing. This RFE adds that feature 
to format a decimal number in a compact format (e.g. 1000 -> 1K, 
100 -> 1M in en_US locale) , which is useful for the environment 
where display space is limited, so that the formatted string can be 
displayed in that limited space. It is defined by LDML's 
specification for Compact Number Formats.


http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats


RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8188147

Request to please help review the the change.

Regards,
Nishit Jain

RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Hi,

Please review this non trivial feature addition to NumberFormat API.

The existing NumberFormat API provides locale based support for 
formatting and parsing numbers which includes formatting decimal, 
percent, currency etc, but the support for formatting a number into a 
human readable or compact form is missing. This RFE adds that feature to 
format a decimal number in a compact format (e.g. 1000 -> 1K, 100 -> 
1M in en_US locale) , which is useful for the environment where display 
space is limited, so that the formatted string can be displayed in that 
limited space. It is defined by LDML's specification for Compact Number 
Formats.


http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats


RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8188147

Request to please help review the the change.

Regards,
Nishit Jain

Re: [12]RFR 8210443: Migrate Locale matching tests to JDK Repo.

[Nishit Jain]

+1

On 24-09-2018 23:04, Naoto Sato wrote:

Looks good to me.

Naoto

On 9/24/18 5:57 AM, Dora Zhou wrote:

Hello,

Please help review the fix for migrating GS-auto Locale matching 
tests to JDK repository. Please refer to the bug description for 
details.


Bug: https://bugs.openjdk.java.net/browse/JDK-8210443

Webrev: http://cr.openjdk.java.net/~dzhou/8210443/webrev.04/ 



Regards,
Dora

[12] RFR 8208560: ChoiceFormat class has unused constants needs cleanup

[Nishit Jain]

Hi,

Please review this cleanup fix for 8208560

Bug: https://bugs.openjdk.java.net/browse/JDK-8208560
Webrev: http://cr.openjdk.java.net/~nishjain/8208560/webrev.00/

Cause and Fix: Changes made for JDK-8021322 has left some unused static 
fields, not sure why these fields have package private access, but they 
are not getting used elsewhere in the package. Removed them from 
ChoiceFormat class.


Regards,
Nishit Jain

[12] RFR 8021322: [Fmt-Ch] Implementation of ChoiceFormat math methods should delegate to java.lang.Math methods

[Nishit Jain]

Hi,

Please review the fix for JDK-8021322. CSR for the spec changes has been 
approved.


Bug: https://bugs.openjdk.java.net/browse/JDK-8021322
Webrev: http://cr.openjdk.java.net/~nishjain/8021322/webrev.01/
CSR: https://bugs.openjdk.java.net/browse/JDK-8208071

Fix: Changed the ChoiceFormat's nextDouble(double), 
previousDouble(double), nextDouble(double, boolean) APIs implementation 
to delegate the execution to the equivalent Math APIs


Regards,
Nishit Jain

[12] RFR: 8193444: SimpleDateFormat throws ArrayIndexOutOfBoundsException when format contains long sequences of unicode characters

[Nishit Jain]

Hi,

Please review the fix for JDK-8193444

Bug: https://bugs.openjdk.java.net/browse/JDK-8193444
Webrev: http://cr.openjdk.java.net/~nishjain/8193444/webrev.02/

Cause: If a format pattern contains a sequence of over 256 non-ASCII 
unicode characters, the length field overflows as it is stored as 8-bit.
Fix: The handling of long_length (> 254) for non-ASCII unicode 
characters is missing in the compilation phase of the format pattern, 
added that in the fix.


Regards,
Nishit Jain

Re: RFR JDK-8204938: Add a test case to automatically check the updated LSR data

[Nishit Jain]

Thanks Naoto, Roger for the review. Made the suggested changes and 
pushed to the repo.


Regards,
Nishit Jain
On 21-06-2018 00:23, Roger Riggs wrote:

Hi Nishit,

Looks ok

My only comment would be to put the relative path to the 
language-subtag-registry.txt in a private static final constant
to call attention to the dependency on the source layout instead of 
burying it in the code.


$.02, Roger


On 6/20/2018 2:27 PM, naoto.s...@oracle.com wrote:

Looks good.

Naoto

On 6/20/18 3:32 AM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-8204938

Bug: https://bugs.openjdk.java.net/browse/JDK-8204938
Webrev: http://cr.openjdk.java.net/~nishjain/8204938/webrev.02/

Fix: Added a test case to cross check the LSR data generated for the 
JDK APIs. So, for each lsr data update, the test case need not be 
updated, it automatically cross checks the updated lsr data.


Regards,
Nishit Jain

RFR JDK-8204938: Add a test case to automatically check the updated LSR data

[Nishit Jain]

Hi,

Please review the fix for JDK-8204938

Bug: https://bugs.openjdk.java.net/browse/JDK-8204938
Webrev: http://cr.openjdk.java.net/~nishjain/8204938/webrev.02/

Fix: Added a test case to cross check the LSR data generated for the JDK 
APIs. So, for each lsr data update, the test case need not be updated, 
it automatically cross checks the updated lsr data.


Regards,
Nishit Jain

RFR JDK-8203872: Upgrading JDK with latest available LSR data from IANA

[Nishit Jain]

Hi,

Please review the fix for JDK-8203872.

Bug: https://bugs.openjdk.java.net/browse/JDK-8203872
Webrev: http://cr.openjdk.java.net/~nishjain/8203872/webrev.01/

Fix: Updated the LSR data to the version 2017-08-15

Regards,
Nishit Jain

[11] RFR JDK-8196399, JDK-8199672: Formatting a decimal using locale-specific grouping..., ClassCastException is thrown...

[Nishit Jain]

Hi,

Please review the fix for JDK-8196399 & JDK-8199672

Bug: https://bugs.openjdk.java.net/browse/JDK-8196399
https://bugs.openjdk.java.net/browse/JDK-8199672
Webrev: http://cr.openjdk.java.net/~nishjain/8196399_8199672/webrev.01/

Issue:
8196399: Locales such as hy_AM do not use grouping, but specify a 
grouping separator. Formatter throws ArithmeticException / by zero while 
identifying the position, as the grouping size is zero.

8199672: Unconditional casting of NumberFormat instance to DecimalFormat

Fix:
8196399: Reset the grouping separator to null character, so that 
grouping separator position identification is skipped.
8199672: Check if the instance returned is DecimalFormat; else, use 
DecimalFormat constructor to obtain the instance.



Regards,
Nishit Jain

Re: [11] RFR JDK-8060094: java/util/Formatter/Basic.java failed in tr locale

[Nishit Jain]

Thanks Naoto,

Please check the updated webrev
http://cr.openjdk.java.net/~nishjain/8060094/webrev.04/

Edits made: In FormatLocale.java, clarified the exception messages about 
the locale used and removed an unused import.


Regards,
Nishit Jain
On 23-02-2018 00:56, Naoto Sato wrote:

Hi Nishit,

In the test case, the exception error messages may be more helpful if 
there is a distinction between the two (line 103 and 120) mentioning 
the formatter is using the default or specified locale.


Naoto

On 2/22/18 3:51 AM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-8060094.

Bug: https://bugs.openjdk.java.net/browse/JDK-8060094
Webrev: http://cr.openjdk.java.net/~nishjain/8060094/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8197916

Cause: The Formatter APIs were not using the correct locale for upper 
casing when specified during instance creation or during format() 
call. The default locale was getting used irrespective of whether a 
Locale is specified in the API.

Fix: Modified the APIs to use the locale specified.

Regards,
Nishit Jain

[11] RFR JDK-8060094: java/util/Formatter/Basic.java failed in tr locale

[Nishit Jain]

Hi,

Please review the fix for JDK-8060094.

Bug: https://bugs.openjdk.java.net/browse/JDK-8060094
Webrev: http://cr.openjdk.java.net/~nishjain/8060094/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8197916

Cause: The Formatter APIs were not using the correct locale for upper 
casing when specified during instance creation or during format() call. 
The default locale was getting used irrespective of whether a Locale is 
specified in the API.

Fix: Modified the APIs to use the locale specified.

Regards,
Nishit Jain

[11] RFR JDK-8190904: Incorrect currency instance returned by java.util.Currency.getInstance()

[Nishit Jain]

Hi,

Please review the fix for JDK-8190904.

Bug: https://bugs.openjdk.java.net/browse/JDK-8190904
Webrev: http://cr.openjdk.java.net/~nishjain/8190904/webrev.07/
CSR: https://bugs.openjdk.java.net/browse/JDK-8196835

Issue: The currency superseding feature gives the possibility of 
allowing two similar Currency instances for a currency code, which is 
against the principle that "There can never more than one Currency 
instance for any given currency".


Fix: Modify the superseding feature to not allow multiple entries with 
same currency code but different numeric and/or minor unit. These are 
ignored as inconsistent entries. Also, If there is any ISO 4217 currency 
data with same currency code as the currency entry given in the 
properties file, then the existing Currency data should be updated with 
the given currency values.



Regards,
Nishit Jain

[10]RFR 8190278: ClassCastException is thrown by java.util.Scanner when a NumberFormatProvider is used.

[Nishit Jain]

Hi,

Please review the fix for JDK-8190278

Bug: https://bugs.openjdk.java.net/browse/JDK-8190278
Webrev: http://cr.openjdk.java.net/~nishjain/8190278/webrev.03/

Fix: Modified the code to check whether the object returned by 
NumberFormat#getNumberInstance(Locale) is a DecimalFormat object, if not,
     then use the DecimalFormat constructor way to create its object, 
where SPI provider implementation is ignored.


Regards,
Nishit Jain

Re: [10] RFR 8187551: MessageFormat.setFormat(int, Format) AIOOBE not thrown when documented

[Nishit Jain]

Thanks Roger,

Updated the webrevto add the new test case in MessageRegression.java

http://cr.openjdk.java.net/~nishjain/8187551/webrev.03/

Regards,
Nishit Jain
On 01-12-2017 20:40, Roger Riggs wrote:

Hi Nishit,

Please add the new test to 
test/jdk/java/text/Format/MessageFormat/MessageRegression.java instead

of creating a new test.

Also the convention for test names should be a functional description 
of the test; bug numbers are uninformative.


(I'm a bit surprised there are few/no regression tests for setFormat).

Thanks, Roger


On 12/1/2017 3:17 AM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-8187551.

Bug: https://bugs.openjdk.java.net/browse/JDK-8187551
Webrev: http://cr.openjdk.java.net/~nishjain/8187551/webrev.02/

Fix: As documented, updated MessageFormat.setFormat() to throw AIOOBE 
with invalid format element index.


Thanks Martin Buchholz for filing the issue and suggesting the fix.

Regards,
Nishit Jain

[10] RFR 8187551: MessageFormat.setFormat(int, Format) AIOOBE not thrown when documented

[Nishit Jain]

Hi,

Please review the fix for JDK-8187551.

Bug: https://bugs.openjdk.java.net/browse/JDK-8187551
Webrev: http://cr.openjdk.java.net/~nishjain/8187551/webrev.02/

Fix: As documented, updated MessageFormat.setFormat() to throw AIOOBE 
with invalid format element index.


Thanks Martin Buchholz for filing the issue and suggesting the fix.

Regards,
Nishit Jain

Re: [10] RFR 6354947: [Fmt-*] DecimalFormat ignores FieldPosition settings on input, contrary to Javadocs

[Nishit Jain]

Thanks Naoto, Roger for the review.

> the summary of the issue does not reflect the issue and implies the 
implementation is wrong; but it is the javadoc that need clarification.
It is probably because when this bug was filed, it was not clear whether 
implementation will be changed as per javadoc or the javadoc will be 
changed.


> Please change the issue summary (on both the CSR and the issue) to 
reflect the true cause.

Done

Regards,
Nishit Jain
On 29-11-2017 20:35, Roger Riggs wrote:

Hi Nishit,

The webrev and updates to the javadoc look fine.

However, the summary of the issue does not reflect the issue and 
implies the implementation is wrong;

but it is the javadoc that need clarification.

Please change the issue summary (on both the CSR and the issue) to 
reflect the true cause, something like:


[Fmt-*] Clarify DecimalFormat description of FieldPosition use

Thanks, Roger


On 11/17/2017 7:21 PM, Naoto Sato wrote:

+1

Naoto

On 11/16/17 10:52 PM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-6354947

Bug: https://bugs.openjdk.java.net/browse/JDK-6354947
Webrev: http://cr.openjdk.java.net/~nishjain/6354947/webrev.02/
CSR: https://bugs.openjdk.java.net/browse/JDK-8191014

Fix: Clarified handling of the FieldPosition settings in the 
java.text.Format APIs specification.


Regards,
Nishit Jain

Re: [10] RFR 8191404: Upgrading JDK with latest available LSR data from IANA.

[Nishit Jain]

Thanks Naoto,

> One nit in the test. At line 71, probably the original intention of 
"str" is to list the accept languages in alphabetic order. Would you 
change it?
Yes, it is sorted according to weight then alphabetically. Updated the 
test case.


http://cr.openjdk.java.net/~nishjain/8191404/webrev.01/

> I have not looked into actual tags in the test. Did you modify them 
manually? If so, it would be desirable to make it automated.
Yes, currently the updated tags are added in the test case manually. 
Making it automated is in the target list, will file/take it as a 
separate issue.
I would like to bring one point to notice that, I have not added all the 
new tags (which are added in this version) in the test case, just few of 
them are added to test that LocaleEquivalentMaps are properly updated. 
Hope that is fine.


Regards,
Nishit Jain
On 22-11-2017 04:47, Naoto Sato wrote:

Hi Nishit,

One nit in the test. At line 71, probably the original intention of 
"str" is to list the accept languages in alphabetic order. Would you 
change it?


I have not looked into actual tags in the test. Did  you modify them 
manually? If so, it would be desirable to make it automated.


Naoto

On 11/16/17 3:47 AM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-8191404

Bug: https://bugs.openjdk.java.net/browse/JDK-8191404
Webrev: http://cr.openjdk.java.net/~nishjain/8191404/webrev.00/

Fix: Updated the LSR data with the version: 2017-08-15

Regards,
Nishit Jain

[10] RFR 6354947: [Fmt-*] DecimalFormat ignores FieldPosition settings on input, contrary to Javadocs

[Nishit Jain]

Hi,

Please review the fix for JDK-6354947

Bug: https://bugs.openjdk.java.net/browse/JDK-6354947
Webrev: http://cr.openjdk.java.net/~nishjain/6354947/webrev.02/
CSR: https://bugs.openjdk.java.net/browse/JDK-8191014

Fix: Clarified handling of the FieldPosition settings in the 
java.text.Format APIs specification.


Regards,
Nishit Jain

[10] RFR 8191404: Upgrading JDK with latest available LSR data from IANA.

[Nishit Jain]

Hi,

Please review the fix for JDK-8191404

Bug: https://bugs.openjdk.java.net/browse/JDK-8191404
Webrev: http://cr.openjdk.java.net/~nishjain/8191404/webrev.00/

Fix: Updated the LSR data with the version: 2017-08-15

Regards,
Nishit Jain

Re: [10] RFR 8180469: Wrong short form text for supplemental Japanese era

[Nishit Jain]

Looks good to me. I am not a reviewer, though.

Regards,
Nishit Jain
On 31-08-2017 04:25, Naoto Sato wrote:

Hi,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8180469

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8180469/webrev.00/

The problem was caused by the difference of the Era display name for 
"SHORT" style between java.time and java.util.Calendar.


Naoto

Re: [10] RFR JDK-8186713: Document default rounding mode in NumberFormat

[Nishit Jain]

Thanks Brian, Naoto for the review. The below updated patch is pushed to 
the repository.


http://cr.openjdk.java.net/~nishjain/8186713/webrev.02/

Regards,
Nishit Jain
On 29-08-2017 04:56, Brian Burkhalter wrote:

Hi Nishit,

I suggest these changes in NumberFormat.java:

184:delete the line
186:s/{@code NumberFormat}/The default implementation/
187-188:s/By default, it uses/It uses/

With these changes I am fine with the patch and no need to send an 
updated webrev unless you prefer.


Thanks,

Brian

On Aug 28, 2017, at 12:43 AM, Nishit Jain <nishit.j...@oracle.com 
<mailto:nishit.j...@oracle.com>> wrote:



>L178-195: Is a list necessary here?
The list approach is used to clearly mention that these two points 
are under "Implementation Requirements". If it does not look good, 
another way could be to use @implSpec tag for individual points, 
separated by . Please check if this is fine.


Also updated the webrev with other suggested changes.

http://cr.openjdk.java.net/~nishjain/8186713/webrev.01/

Re: [10] RFR JDK-8186713: Document default rounding mode in NumberFormat

[Nishit Jain]

Hi Brian,

> L178-195: Is a list necessary here?
The list approach is used to clearly mention that these two points are 
under "Implementation Requirements". If it does not look good, another 
way could be to use @implSpec tag for individual points, separated by 
. Please check if this is fine.


Also updated the webrev with other suggested changes.

http://cr.openjdk.java.net/~nishjain/8186713/webrev.01/

Regards,
Nishit Jain
On 25-08-2017 00:55, Brian Burkhalter wrote:

A few minor comments:

L178-195: Is a list necessary here?

L186: I don’t think “Rounding: " is necessary.

L187: s/the numbers/numbers/

L188: s/uses/uses the/

L191: s/its/the/

Thanks,

Brian

On Aug 24, 2017, at 12:06 AM, Nishit Jain <nishit.j...@oracle.com 
<mailto:nishit.j...@oracle.com>> wrote:


Webrev:http://cr.openjdk.java.net/~nishjain/8186713/webrev.00/ 
<http://cr.openjdk.java.net/%7Enishjain/8186713/webrev.00/>

[10] RFR JDK-8186713: Document default rounding mode in NumberFormat

[Nishit Jain]

Hi,

Please review the fix for JDK-8186713

Bug: https://bugs.openjdk.java.net/browse/JDK-8186713
Webrev: http://cr.openjdk.java.net/~nishjain/8186713/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8185777

Fix: Updated the specification of NumberFormat about the default 
rounding mode used while formatting the numbers. Also, in the 
specification of java.util.Formatter, changed the reference of the 
deprecated BigDecimal.ROUND_HALF_UP,with java.math.RoundingMode.HALF_UP.


Regards,
Nishit Jain