Re: [13] RFR: 8218948: SimpleDateFormat :: format - Zone Names are not reflected correctly during run time
Hi Naoto, This fix looks good to me. Thanks, Rachna On 3/6/19 4:50 AM, Naoto Sato wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8218948 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8218948/webrev.00/ This is a follow on fix to 8217366. There are several root causes included in this bug, when the runtime supplements missing display names in non-US locales :- - Retrieve the names for the requested id first, before it maps to the canonical id, and on canonicalizing the id, use unique way (canonicalTZID()) to canonicalize. - Retrieve the names in the same manner, between DateFormatSymbols.getZoneStrings() and TimeZone.getDisplayName(). - Correctly map the Chinese locales between JDK and CLDR, in terms of Simplified/Traditional scripts. Naoto -- Thanks, Rachna
Re: [12] RFR: 8216176: Clarify the singleton description in j.t.c.JapaneseEra class
Looks good to me. On 1/8/19 11:16 PM, Naoto Sato wrote: Hello, Please review the change for the following issue: Issue: https://bugs.openjdk.java.net/browse/JDK-8216176 CSR: https://bugs.openjdk.java.net/browse/JDK-8216177 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8216176/webrev.00/ This is a simple one line change that is clarifying the era singleton description, which was added with the fix to 8212941. Naoto -- Thanks, Rachna
Re: [13] RFR 8215913: [Test_bug]java/util/Locale/LocaleProvidersRun.java failed on de_DE and ja_JP locale.
Looks good to me. Thanks, Rachna On 1/7/19 12:36 PM, Dora Zhou wrote: Hi Rachna, Updated the webrev: http://cr.openjdk.java.net/~dzhou/8215913/webrev.01/ Regards, Dora On 2019/1/4 17:44, Rachna Goel wrote: Hi Dora, Kindly update copyright years in both files and add bug id in LocaleProvidersRun.java. Other than that, it looks good to me. Thanks, Rachna On 1/4/19 7:58 AM, Dora Zhou wrote: Hello, Please help review the fix for the test bug java/util/Locale/LocaleProvidersRun.java failed on de_DE and ja_JP locale. Bug: https://bugs.openjdk.java.net/browse/JDK-8215913 Webrev: http://cr.openjdk.java.net/~dzhou/8215913/webrev.00/ Thanks, Dora -- Thanks, Rachna -- Thanks, Rachna
Re: [12] RFR: 8215303: Allowing additional currency code points from later Unicode updates
Hi Naoto, just one nit, copyright year need to be updated in Character.java. Thanks, Rachna On 1/3/19 10:26 PM, 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 -- Thanks, Rachna
Re: [12] RFR: 8215303: Allowing additional currency code points from later Unicode updates
Hi Naoto, Your fix looks good to me. Thanks, Rachna On 1/3/19 10:26 PM, 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 -- Thanks, Rachna
Re: [13] RFR 8215913: [Test_bug]java/util/Locale/LocaleProvidersRun.java failed on de_DE and ja_JP locale.
Hi Dora, Kindly update copyright years in both files and add bug id in LocaleProvidersRun.java. Other than that, it looks good to me. Thanks, Rachna On 1/4/19 7:58 AM, Dora Zhou wrote: Hello, Please help review the fix for the test bug java/util/Locale/LocaleProvidersRun.java failed on de_DE and ja_JP locale. Bug: https://bugs.openjdk.java.net/browse/JDK-8215913 Webrev: http://cr.openjdk.java.net/~dzhou/8215913/webrev.00/ Thanks, Dora -- Thanks, Rachna
Re: [12] RFR: 8215194: Initial size of UnicodeBlock map is incorrect
Hi Naoto, Thanks for fixing this. Your fix looks good to me. Thanks, Rachna On 12/11/18 8:21 PM, Naoto Sato wrote: Hi, Please review the fix for the following issue: https://bugs.openjdk.java.net/browse/JDK-8215194 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8215194/webrev.00/ This one line fix is for the correctness of the initial map size of Character.UnicodeBlock. Naoto -- Thanks, Rachna
RFR: [12] JDK-8209923 : Unicode 11.0.0
Hi, Kindly review this enhancement to support Unicode version in JDK to 11.0.0. Bug: https://bugs.openjdk.java.net/browse/JDK-8209923 patch: http://cr.openjdk.java.net/~rgoel/JDK-8209923/webrev.02/ -- Thanks, Rachna
[12] RFR:8210490:TimeZone.getDisplayName given Locale.US doesn't always honor the Locale
Hi, Kindly review fix to JDK-8210490. Bug: https://bugs.openjdk.java.net/browse/JDK-8210490 Webrev: http://cr.openjdk.java.net/~rgoel/JDK-8210490/webrev.05/ This is a regression caused by JDK-8202537, where for timezones such as Etc/GMT-5, display names got formatted according to default locale. Fix is to specify locale, which was specified in timezone.getDisplayName() method. -- Thanks, Rachna
[11] RFR: 8209047:"Illegal pattern character 'B'" IllegalArgumentException with Burmese locales
Hi, Kindly review the fix to JDK-8209047. Bug: https://bugs.openjdk.java.net/browse/JDK-8209047 webrev: http://cr.openjdk.java.net/~rgoel/JDK-8209047/webrev.03/ It is a regression caused by JDK-8202537. Because of the 'B' character introduced in the CLDR time patterns "B HH:mm:ss", "B H:mm" (where B represents the 'day period') for "my" (Burmese) locale. Since, this character is not supported in java.text.SimpleDateFormat and in java.time.DateTimeFormatter, it is throwing IllegalArgumentException. Fix is to change time pattern to previous CLDR's version i.e 29, until 'B' character is supported via JDK-8209175. -- Thanks, Rachna
[11]RFR:JDK-8206965: java/util/TimeZone/Bug8149452.java failed on de_DE and ja_JP locale.
Hi, Kindly review fix for JDK-8206965. Bug: https://bugs.openjdk.java.net/browse/JDK-8206965 patch: http://cr.openjdk.java.net/~rgoel/JDK-8206965/webrev.06/ This is a regression caused by JDK-8181157 due to which for some locales, timezone display names were not getting generated. Generating fallback names for missing timezones at runtime would solve this issue. -- Thanks, Rachna
[11] Review Request: 8204603: Short week days, NaN value and timezone name are inconsistent between CLDR and Java in zh_CN, zh_TW locales.
Hi, Kindly review fix to JDK-8204603. Bug: https://bugs.openjdk.java.net/browse/JDK-8204603 Fix: http://cr.openjdk.java.net/~rgoel/JDK-8204603/webrev.04/ This is a regression caused by JDK-8179071, where locale data is getting incorrectly retrieved (not following languageAliases of CLDR) for some locales. Handling of languageAliases for CLDR provider will solve the issue. -- Thanks, Rachna
Re: RFR: JDK-8205158: Update the .md files for 3rd party software Unicode 10.0, ICU 60.1, and CLDR v33
Hi Mandy, Thanks for the review. Unicode 10.0.0 and ICU 60.2 were integrated with : https://bugs.openjdk.java.net/browse/JDK-8191410 and CLDR 33 was: https://bugs.openjdk.java.net/browse/JDK-8202537 Linked mentioned issues with this one. Thanks, Rachna On 6/22/18 9:57 PM, mandy chung wrote: On 6/22/18 6:54 AM, Rachna Goel wrote: Hi, Kindly review fix to update legal files for Unicode, CLDR and ICU. Issue: https://bugs.openjdk.java.net/browse/JDK-8205158 Patch :http://cr.openjdk.java.net/~rgoel/JDK-8205158/webrev.02/ Looks okay. What are the issues that upgrades these libraries? Can you link them with JDK-8205158? Mandy -- Thanks, Rachna
Re: RFR: JDK-8205158: Update the .md files for 3rd party software Unicode 10.0, ICU 60.1, and CLDR v33
Hi, Kindly ignore patch mentioned in previous mail. patch for review is : http://cr.openjdk.java.net/~rgoel/JDK-8205158/webrev.01/ Thanks, Rachna On 6/22/18 7:24 PM, Rachna Goel wrote: Hi, Kindly review fix to update legal files for Unicode, CLDR and ICU. Issue: https://bugs.openjdk.java.net/browse/JDK-8205158 Patch :http://cr.openjdk.java.net/~rgoel/JDK-8205158/webrev.02/ -- Thanks, Rachna
RFR: JDK-8205158: Update the .md files for 3rd party software Unicode 10.0, ICU 60.1, and CLDR v33
Hi, Kindly review fix to update legal files for Unicode, CLDR and ICU. Issue: https://bugs.openjdk.java.net/browse/JDK-8205158 Patch :http://cr.openjdk.java.net/~rgoel/JDK-8205158/webrev.02/ -- Thanks, Rachna
Re: RFR: [11] 8202537: CLDR 33
Hi Naoto, Thanks a lot for the review. I have made suggested changes, Kindly have a look at : http://cr.openjdk.java.net/~rgoel/JDK-8202537/webrev.07/ - Updated NumberingSystemsParseHandler.java - Updated LocaleData.cldr for new test case. Thanks, Rachna On 6/13/18 10:33 PM, naoto.s...@oracle.com wrote: Hi Rachna, A couple of comments: - NumberingSystemsParseHandler.java Since the code substitutes latin digits for supplementary digits, it can skip line 68-79. - A test should be written for the above substitution. Naoto On 6/12/18 10:33 PM, Rachna Goel wrote: Hi, Kindly review fix for JDK-8202537. Fix is to upgrade Unicode consortium's CLDR data into JDK from current version 29 to 33. For more info : http://cldr.unicode.org/index/downloads/cldr-33 Bug : https://bugs.openjdk.java.net/browse/JDK-8202537 Patch: http://cr.openjdk.java.net/~rgoel/JDK-8202537/webrev.06/index.html -- Thanks, Rachna
Re: RFR: [11] 8202537: CLDR 33
Hi Roger, No update is done mechanically between CLDR data and .xml files. CLDR's data in .xml files are generated into resourcebundles at build time by cldrconverter tool. Already existing regression test "test/jdk/sun/text/resources/LocaleDataTest.java" verify that same data is retrieved by APIs. Thanks, Rachna* * On 6/13/18 8:06 PM, Roger Riggs wrote: Hi Rachna, How much of the updates are done mechanically between the CLDR data and the .xml files? Manually reviewing that many files is likely to be error prone. Thanks, Roger On 6/13/2018 1:33 AM, Rachna Goel wrote: Hi, Kindly review fix for JDK-8202537. Fix is to upgrade Unicode consortium's CLDR data into JDK from current version 29 to 33. For more info : http://cldr.unicode.org/index/downloads/cldr-33 Bug : https://bugs.openjdk.java.net/browse/JDK-8202537 Patch: http://cr.openjdk.java.net/~rgoel/JDK-8202537/webrev.06/index.html -- Thanks, Rachna
RFR: [11] 8202537: CLDR 33
Hi, Kindly review fix for JDK-8202537. Fix is to upgrade Unicode consortium's CLDR data into JDK from current version 29 to 33. For more info : http://cldr.unicode.org/index/downloads/cldr-33 Bug : https://bugs.openjdk.java.net/browse/JDK-8202537 Patch: http://cr.openjdk.java.net/~rgoel/JDK-8202537/webrev.06/index.html -- Thanks, Rachna
[11] RFR JDK-8203474: Update description of "Cyrillic Supplementary" block name in Character.UnicodeBlock class.
Hi, Kindly review this small doc fix to Character.UnicodeBlock class. Bug: https://bugs.openjdk.java.net/browse/JDK-8203474 Patch: --- a/src/java.base/share/classes/java/lang/Character.javaSun May 27 12:00:16 2018 +0530 +++ b/src/java.base/share/classes/java/lang/Character.java Mon May 28 11:48:58 2018 +0530 @@ -1419,7 +1419,8 @@ "YIRADICALS"); /** - * Constant for the "Cyrillic Supplementary" Unicode character block. + * Constant for the "Cyrillic Supplement" Unicode character block. + * This block was previously known as the "Cyrillic Supplementary" block. * @since 1.5 */ CSR : https://bugs.openjdk.java.net/browse/JDK-8203475 -- Thanks, Rachna
[11] RFR: JDK-8202582 : DateTimeFormatterBuilder.parseOffsetBased unnecessarily calls toString()
Hi, Kindly review this small patch for JDK-8202582. https://bugs.openjdk.java.net/browse/JDK-8202582 http://cr.openjdk.java.net/~rgoel/JDK-8202582/webrev/ Fix is to call text.subSequence() before calling toString() on input string, for more performance. -- Thanks, Rachna
Re: [11] RFR:JDK-8179071:Month value is inconsistent between CLDR and Java in some special locales
One correction: E.g "pa-PK" wil be replaced by "pa-Arab-PK", as former is a deprecated/legacy tag. On 4/27/18 12:05 PM, Rachna Goel wrote: Hi, Kindly review the fix for JDK-8179071. Bug : https://bugs.openjdk.java.net/browse/JDK-8179071 Patch: http://cr.openjdk.java.net/~rgoel/JDK-8179071/webrev.03/index.html Fix is to parse and store language aliases( Deprecated and Legacy ) from CLDR's SupplemetalMetaData file. At run time, If a deprectaed/legacy locale tag is found, It will be replaced by its replacement tag. E.g "pa-PK" will be replaced by "pa-Guru-PK". -- Thanks, Rachna
[11] RFR:JDK-8179071:Month value is inconsistent between CLDR and Java in some special locales
Hi, Kindly review the fix for JDK-8179071. Bug : https://bugs.openjdk.java.net/browse/JDK-8179071 Patch: http://cr.openjdk.java.net/~rgoel/JDK-8179071/webrev.03/index.html Fix is to parse and store language aliases( Deprecated and Legacy ) from CLDR's SupplemetalMetaData file. At run time, If a deprectaed/legacy locale tag is found, It will be replaced by its replacement tag. E.g "pa-PK" will be replaced by "pa-Guru-PK". -- Thanks, Rachna
Re: [11] RFR: 8191410 : Unicode 10.0.0
Hi Roger, Ivan,
There is no change in algorithm as such but there has been mainly
stability improvements, defects fixed, performance enhancements. e.g
Updated header check, version and loading for latest binary files,
Simple case mappings will be more efficient as unnecessary checking has
been removed.
complete list of changes can be found at:
http://bugs.icu-project.org/trac/query?status=closed=fixed=fixedbyotherticket=60.1=60.2=project=999=id=summary=resolution=milestone=status=owner=type=priority=project=weeks=priority
Also, I have incorporated suggestions given from Ivan.
Kindly have a look at updated webrev:
http://cr.openjdk.java.net/~rgoel/JDK-8191410/webrev.01/
Thanks,
Rachna
On 3/8/18 9:47 PM, Roger Riggs wrote:
Hi Rachna,
sun/text/normalizer/NormalizerImpl.java:
Is there a higher level description of the algorithmic changes?
102-105: These look like accidental changes: the space should not be
removed and the trailing "{" doesn't make sense in a comment.
Regards, Roger
On 3/8/2018 6:56 AM, Rachna Goel wrote:
Hi,
Please review the proposed changes for JDK-819410.
Bug : https://bugs.openjdk.java.net/browse/JDK-8191410
proposed changeset is located at :
http://cr.openjdk.java.net/~rgoel/JDK-8191410/webrev/
This serves as the implementation for JEP 327.
--
Thanks,
Rachna
[11] RFR: 8191410 : Unicode 10.0.0
Hi, Please review the proposed changes for JDK-819410. Bug : https://bugs.openjdk.java.net/browse/JDK-8191410 proposed changeset is located at : http://cr.openjdk.java.net/~rgoel/JDK-8191410/webrev/ This serves as the implementation for JEP 327. -- Thanks, Rachna
Re: [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols
Hi,
Kindly review updated patch for this doc fix:
patch: http://cr.openjdk.java.net/%7Ergoel/8146656/webrev.02/
Approved CSR : https://bugs.openjdk.java.net/browse/JDK-8191414
Thanks,
Rachna
On 20/12/17 10:44 PM, joe darcy wrote:
Hi Rachna,
I think the revised version with the @implSpec tag switch is
acceptable, but also think providing more text to describe this
situation would be helpful to programmers unaware of a 13 month
possibility.
Cheers,
-Joe
On 12/19/2017 2:08 AM, Rachna Goel wrote:
Hi Joe,
Thanks for the comments.
I have updated the CSR to have @implSpec in place of @implNote.
https://bugs.openjdk.java.net/browse/JDK-8191414
Regarding "An array with either 12 or 13 elements will be returned
depending on whether or {@link Calendar.UNDECIMBER} is supported." ,
I would like to go with existing statement as this method always
returns 13 elements where the 13th element may be empty string or may
contain Calendar.UNDECIMBER, depending upon whether its supported by
the Calendar instance.
kindly suggest whether this looks fine!
Thanks,
Rachna
On 19/12/17 2:55 PM, joe darcy wrote:
Hi Rachna,
On 12/19/2017 1:13 AM, Rachna Goel wrote:
Hello Joe,
Thanks for the review.
Reason I added @implNote is that it's the case for the default
implementation. Not added as a part of spec, as some implementation
can just return 12 element array for same methods through the
"java.text.spi.DateFormatSymbolsProvider" SPI.
That is precisely the sort of situation the @implSpec tag is
intended for. It allows the specification to say DateFormatSymbols
must behave this way while allowing subclasses to behave differently.
Perhaps some general text can be added as normal specification,
something like
"An array with either 12 or 13 elements will be returned depending
on whether or {@link Calendar.UNDECIMBER} is supported."
paired with
@implSpec This method returns 13 elements since @link
Calendar.UNDECIMBER} is supported.
HTH,
-Joe
--
Thanks,
Rachna
--
Thanks,
Rachna
Re: [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols
Hi Joe,
I have revised this webrev to include your feedback and also updated
this CSR.
Kindly have a look at :
CSR: https://bugs.openjdk.java.net/browse/JDK-8191414
webrev: http://cr.openjdk.java.net/~rgoel/8146656/webrev.01/
Thanks,
Rachna
On 20/12/17 10:44 PM, joe darcy wrote:
Hi Rachna,
I think the revised version with the @implSpec tag switch is
acceptable, but also think providing more text to describe this
situation would be helpful to programmers unaware of a 13 month
possibility.
Cheers,
-Joe
On 12/19/2017 2:08 AM, Rachna Goel wrote:
Hi Joe,
Thanks for the comments.
I have updated the CSR to have @implSpec in place of @implNote.
https://bugs.openjdk.java.net/browse/JDK-8191414
Regarding "An array with either 12 or 13 elements will be returned
depending on whether or {@link Calendar.UNDECIMBER} is supported." ,
I would like to go with existing statement as this method always
returns 13 elements where the 13th element may be empty string or may
contain Calendar.UNDECIMBER, depending upon whether its supported by
the Calendar instance.
kindly suggest whether this looks fine!
Thanks,
Rachna
On 19/12/17 2:55 PM, joe darcy wrote:
Hi Rachna,
On 12/19/2017 1:13 AM, Rachna Goel wrote:
Hello Joe,
Thanks for the review.
Reason I added @implNote is that it's the case for the default
implementation. Not added as a part of spec, as some implementation
can just return 12 element array for same methods through the
"java.text.spi.DateFormatSymbolsProvider" SPI.
That is precisely the sort of situation the @implSpec tag is
intended for. It allows the specification to say DateFormatSymbols
must behave this way while allowing subclasses to behave differently.
Perhaps some general text can be added as normal specification,
something like
"An array with either 12 or 13 elements will be returned depending
on whether or {@link Calendar.UNDECIMBER} is supported."
paired with
@implSpec This method returns 13 elements since @link
Calendar.UNDECIMBER} is supported.
HTH,
-Joe
--
Thanks,
Rachna
--
Thanks,
Rachna
Re: [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols
Hi Joe,
Thanks for the comments.
I have updated the CSR to have @implSpec in place of @implNote.
https://bugs.openjdk.java.net/browse/JDK-8191414
Regarding "An array with either 12 or 13 elements will be returned
depending on whether or {@link Calendar.UNDECIMBER} is supported." , I
would like to go with existing statement as this method always returns
13 elements where the 13th element may be empty string or may contain
Calendar.UNDECIMBER, depending upon whether its supported by the
Calendar instance.
kindly suggest whether this looks fine!
Thanks,
Rachna
On 19/12/17 2:55 PM, joe darcy wrote:
Hi Rachna,
On 12/19/2017 1:13 AM, Rachna Goel wrote:
Hello Joe,
Thanks for the review.
Reason I added @implNote is that it's the case for the default
implementation. Not added as a part of spec, as some implementation
can just return 12 element array for same methods through the
"java.text.spi.DateFormatSymbolsProvider" SPI.
That is precisely the sort of situation the @implSpec tag is intended
for. It allows the specification to say DateFormatSymbols must behave
this way while allowing subclasses to behave differently.
Perhaps some general text can be added as normal specification,
something like
"An array with either 12 or 13 elements will be returned depending on
whether or {@link Calendar.UNDECIMBER} is supported."
paired with
@implSpec This method returns 13 elements since @link
Calendar.UNDECIMBER} is supported.
HTH,
-Joe
--
Thanks,
Rachna
Re: [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols
Hello Joe, Thanks for the review. Reason I added @implNote is that it's the case for the default implementation. Not added as a part of spec, as some implementation can just return 12 element array for same methods through the "java.text.spi.DateFormatSymbolsProvider" SPI. Thanks, Rachna On 19/12/17 2:07 PM, joe darcy wrote: Hello Rachna, On 12/18/2017 10:35 PM, Rachna Goel wrote: Hi, Kindly review API Doc fix for java.text.DateFormatSymbols. JBS Issue : https://bugs.openjdk.java.net/browse/JDK-8146656 Webrev: http://cr.openjdk.java.net/~rgoel/8146656/webrev/ CSR: https://bugs.openjdk.java.net/browse/JDK-8191414 An addendum to my CSR review. The newly added text should be normative, not just informative. That is, the text should officially be part of the specification of the class. If you do not want the 13 elements behavior to be required of a subclass, change the @implNote into an @implSpec. If you want 13 elements to be required of subclasses too, replace @implNote with a paragraph begin. (For more guidance on @impNote vs @implspec, etc. see http://openjdk.java.net/jeps/8068562) The CSR should be updated with the amended API change. Thanks, -Joe -- Thanks, Rachna
[11] RFR of 8146656: Wrong Months Array for DateFormatSymbols
Hi, Kindly review API Doc fix for java.text.DateFormatSymbols. JBS Issue : https://bugs.openjdk.java.net/browse/JDK-8146656 Webrev: http://cr.openjdk.java.net/~rgoel/8146656/webrev/ CSR: https://bugs.openjdk.java.net/browse/JDK-8191414 -- Thanks, Rachna
JDK10 RFR of JDK-8185841:Values from getFirstDayOfWeek() are inconsistent with CLDR
Hi, Kindly review the fix to JDK-8185841. Bug : https://bugs.openjdk.java.net/browse/JDK-8185841. patch : http://cr.openjdk.java.net/~rgoel/JDK-8185841/webrev.08/ Fix is to relocate this region dependent data. New Bundle will be generated when Locale id has non empty script and country code and it contains region dependent data. e.g for "az_Latn_AZ" locale, CalendarData_az_AZ.java Bundle will be generated and the data currently in CalendarData_az_Latn_AZ.java should be relocated into it. -- Thanks, Rachna
Re: RFR(s): 8171958: Several tests from java/time/test/java/time/format requiring jdk.localedata for execution
Hi Sergei, Just for curiosity, what is the purpose of moving package statement down. Sorry for pointing out, Copyright year needs to be updated to 2017. Thanks, Rachna On 09/01/17 3:15 PM, Sergei Kovalev wrote: Hi All, Re-sending request because I'm still looking for reviewers. In addition for TEST.properties modification I'd like to put citation from Jon G. email. FWIW, I note your TEST.properties file looks malformed. It has 2 modules entries (line 4, line 7), which by the standard rules of Java properties files, means that "last one wins". line source # Threeten test uses TestNG TestNG.dirs = . othervm.dirs = tck/java/time/chrono test/java/time/chrono test/java/time/format modules = jdk.localedata lib.dirs = ../../lib/testlibrary lib.build = jdk.testlibrary.RandomFactory modules = java.base/java.time:open java.base/java.time.chrono:open java.base/java.time.zone:open This mean that first modules declaration could be safely removed as I did. 28.12.16 17:20, Sergei Kovalev wrote: Hi Rachna, Thank you for the comment. I've reviewed usage of module declaration in TEST.properties file and find that it could be removed without any impact. In both cases (with original and modified TEST.properties file) I get same result. Test results: passed: 142; failed: 27 The reason of this behavior is the lack of jtreg header in test sources. In case test source has no "@test" tag jtreg ignores all properties that exists in TEST.properties file. I recreated the review by adding TEST.properties modification: http://cr.openjdk.java.net/~skovalev/8171958/webrev.01/ 27.12.16 13:55, Rachna Goel wrote: Hi Sergei, Though I am not a reviewer, But I have one comment on this fix. test/java/time/TEST.properties declares "modules = jdk.localedata" , so that all tests for java.time can have access to "jdk.localedata" module. If we are restricting usage of jdk.localedata module for different tests, then TEST.properties need to be updated as well. Thanks, Rachna On 26/12/16 8:27 PM, Sergei Kovalev wrote: Hello Team, Please review below fix for tests. Bug ID: https://bugs.openjdk.java.net/browse/JDK-8171958 Web review: http://cr.openjdk.java.net/~skovalev/8171958/webrev.00/ Issue: some tests fails in case of module limitation by '--limit-module java.base' command line option. Root cause: The tests uses locale data that stored in separate resource file "jdk.localedata". Solution: Add declaration of required module. In same cases a test file contains many test items, part of which could be executed with java.base module only. In this cases we can separate the items and extract that items which depend on locale data and run them individually. Therefore this proposal contains additional test files where added "WithLocale" suffix which demonstrate dependency on resources. Alternative solution could be add a required module declaration "jdk.localedata" to all files. However this will lead to unnecessary test coverage reduction. -- With best regards, Sergei -- Thanks, Rachna
Re: RFR(s): 8171958: Several tests from java/time/test/java/time/format requiring jdk.localedata for execution
Hi Sergei, Though I am not a reviewer, But I have one comment on this fix. test/java/time/TEST.properties declares "modules = jdk.localedata" , so that all tests for java.time can have access to "jdk.localedata" module. If we are restricting usage of jdk.localedata module for different tests, then TEST.properties need to be updated as well. Thanks, Rachna On 26/12/16 8:27 PM, Sergei Kovalev wrote: Hello Team, Please review below fix for tests. Bug ID: https://bugs.openjdk.java.net/browse/JDK-8171958 Web review: http://cr.openjdk.java.net/~skovalev/8171958/webrev.00/ Issue: some tests fails in case of module limitation by '--limit-module java.base' command line option. Root cause: The tests uses locale data that stored in separate resource file "jdk.localedata". Solution: Add declaration of required module. In same cases a test file contains many test items, part of which could be executed with java.base module only. In this cases we can separate the items and extract that items which depend on locale data and run them individually. Therefore this proposal contains additional test files where added "WithLocale" suffix which demonstrate dependency on resources. Alternative solution could be add a required module declaration "jdk.localedata" to all files. However this will lead to unnecessary test coverage reduction.
Re: RFR: JDK-8075577: java.time does not support HOST provider
Hi Roger, Thanks for the review. Updated patch is : http://cr.openjdk.java.net/~rgoel/JDK-8075577/webrev.02/ Please find some of my comments inlined. On 30/11/16 10:17 PM, Roger Riggs wrote: Hi Rachna, macosx//HostLocaleProviderAdapterImpl.java: - line 172-177, might be a good place to use the new ConcurrentMap.computeIfAbsent I could have replaced those lines with : dateFormatPatternsMap.computeIfAbsent(locale, k -> new SoftReference<>(new AtomicReferenceArray<>(5 * 5))); But, there are two check made on line no 173. (ref == null ) which will be checked by computeIfAbsent, But about second (dateFormatPatterns = ref.get()) == null) will not be checked, if those lines replaced. - line 197: you could pre-size the StringBuilder since the target length can be estimated (unless they are all less than the default 16) pre-sized it with initial " jrepattern" string. macosx && windows//HostLocaleProviderAdapterImpl.java - toJavaTimeDateTimePattern() - is there a way to avoid having two copies of this function? Perhaps as a static method in JavaTimeDateTimePatternImpl.java. There seems to be no way to avoid having two copies. Implementations of "HostLocaleProviderAdapterImpl" for different HOST Environments are loaded at run time by HOSTLocaleProviderAdapter. JavaTimeDateTimePatternImpl is an implementation of "JavaTimeDateTimePatternProvider" for JRE LocaleProviderAdapter. The noreg-hard label on issue indicates testing is difficult and specific to OS and host configuration but it also seems unusual to have this much code and not have a regression test. I am in touch with I18n SQE on writing tests for HOST Providers. But testing scope, Golden data are yet to be discussed. If Masayoshi is satisfied and you have tested it in the target configuration then perhaps it is not worthwhile to invest in a special case regression test. Thanks, Roger On 11/30/2016 4:39 AM, Masayoshi Okutsu wrote: Looks good to me. Masayoshi On 11/22/2016 6:30 PM, Rachna Goel wrote: Hi, Please review fix for JDK-8075577. Bug : https://bugs.openjdk.java.net/browse/JDK-8075577 webrev : http://cr.openjdk.java.net/~rgoel/JDK-8075577/webrev.01/ Fix is to introduce new private spi "sun.text.spi.JavaTimeDateTimePatternProvider.java" to retrieve LocaleProvider specific Date/Time Patterns for "java.time" . Thanks, Rachna
RFR: JDK-8075577: java.time does not support HOST provider
Hi, Please review fix for JDK-8075577. Bug : https://bugs.openjdk.java.net/browse/JDK-8075577 webrev : http://cr.openjdk.java.net/~rgoel/JDK-8075577/webrev.01/ Fix is to introduce new private spi "sun.text.spi.JavaTimeDateTimePatternProvider.java" to retrieve LocaleProvider specific Date/Time Patterns for "java.time" . Thanks, Rachna
Re: RFR:JDK-8168906-Tighten permissions granted to the jdk.localedata module
sorry, correction to typo As jdk.localedata module does *not* read any system properties, tightened permissions for same. On 14/11/16 12:42 PM, Rachna Goel wrote: Hi, Kindly review fix for JDK-8168906. https://bugs.openjdk.java.net/browse/JDK-8168906 Patch :http://cr.openjdk.java.net/~rgoel/JDK-8168906/webrev/ fix: As jdk.localedata module does read any system properties, tightened permissions for same. Thanks, Rachna
RFR:JDK-8168906-Tighten permissions granted to the jdk.localedata module
Hi, Kindly review fix for JDK-8168906. https://bugs.openjdk.java.net/browse/JDK-8168906 Patch :http://cr.openjdk.java.net/~rgoel/JDK-8168906/webrev/ fix: As jdk.localedata module does read any system properties, tightened permissions for same. Thanks, Rachna
RFR: JDK-8146750:java.time.Month.getDisplayName() return incorrect narrow names with JRE provider on locale de,de_DE,en_US.
Hi, Please review fix for JDK-8146750. Bug : https://bugs.openjdk.java.net/browse/JDK-8146750 webrev : http://cr.openjdk.java.net/~rgoel/JDK-8146750/webrev.09/ Fix is to retrieve Narrow and Narrow_Standalone Month names and Day names one by one, as they can be duplicate. Thanks, Rachna
RFR:JDK-8146750:java.time.Month.getDisplayName() return incorrect narrow names with JRE provider.
Hi, Please review fix for JDK-8146750. Bug : https://bugs.openjdk.java.net/browse/JDK-8146750 Fix: http://cr.openjdk.java.net/~rgoel/JDK-8146750/webrev.04/ Fix is to retrieve Narrow and Narrow_Standalone Month names as well as Day names one by one, as they can be duplicate. Thanks, Rachna
