Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

Hi Roger, Thank you for your review and suggestions. On 7/15/20 10:56 AM, Roger Riggs wrote: Hi Naoto, Given the extra tests in the body of the loop, I think its worth finding or creating a JMH test for this and checking the performance. Good point. With performance in mind, I would

Re: RFR: 8248655: Support supplementary characters in String case insensitive operations

Thank you, Jim, for the quick review! On 7/15/20 9:26 AM, Jim Laskey wrote: I think I'm good with this. +1 Asides: 325 int cp1 = (int)getChar(value, k1); 326 int cp2 = (int)getChar(other, k2); I would be tempted to short cut by exiting when not equal, but I think

RFR: 8248655: Support supplementary characters in String case insensitive operations

Hello, Please review the fix to the following issues: https://bugs.openjdk.java.net/browse/JDK-8248655 https://bugs.openjdk.java.net/browse/JDK-8248434 The proposed changeset and its CSR are located at: https://cr.openjdk.java.net/~naoto/8248655.8248434/webrev.00/

Re: RFR: 8248695: HostLocaleProviderAdapterImpl provides invalid date-only

Hi Joe, On 7/13/20 7:28 PM, Joe Wang wrote: On 7/13/2020 7:01 PM, naoto.s...@oracle.com wrote: Hi Joe, Thank you for your review. On 7/13/20 3:55 PM, Joe Wang wrote: Hi Naoto, Would it make sense to provide an additional test using the public APIs similar to the one provided in the bug

Re: RFR: 8248695: HostLocaleProviderAdapterImpl provides invalid date-only

Hi Joe, Thank you for your review. On 7/13/20 3:55 PM, Joe Wang wrote: Hi Naoto, Would it make sense to provide an additional test using the public APIs similar to the one provided in the bug report? I'm sure yours is correct and covers more cases than the original, but it would be nice to

Re: RFR: 8248695: HostLocaleProviderAdapterImpl provides invalid date-only

Ping. On 7/7/20 3:55 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8248695 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8248695/webrev.00/ There were two causes that resulted in

Re: RFR: 8249086: JDK 15 L10n resource file update - msg drop 10

Hi Leo, Looks good to me. Naoto On 7/8/20 10:58 AM, li.ji...@oracle.com wrote: Hi, Pls review the l10n resource files update for JDK 15 msg drop 10. Bug: https://bugs.openjdk.java.net/browse/JDK-8249086 Webrev: http://cr.openjdk.java.net/~ljiang/8249086/read/ Thanks, Leo

RFR: 8248695: HostLocaleProviderAdapterImpl provides invalid date-only

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8248695 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8248695/webrev.00/ There were two causes that resulted in throwing exceptions. One was that the Host adapter for

Re: RFR 8248412: test/jdk/java/sql/testng/test/sql/DriverManagerPermissionsTests.java can fail

Hi Lance, The change looks good to me. Naoto On 6/26/20 8:25 AM, Lance Andersen wrote: Hi all, Please review this patch for https://bugs.openjdk.java.net/browse/JDK-8248412 where DriverManagerPermissionsTests.java can occasionally fail in

Re: RFR 8248184: AMPM_OF_DAY doc fix in ChronoField

Thanks, Brian and Lance. I forgot to update the copyright year. Pushed the changeset with the year update. Naoto On 6/23/20 3:09 PM, Lance Andersen wrote: +1 Best Lance -- Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1

RFR 8248184: AMPM_OF_DAY doc fix in ChronoField

Hi, Please review this small doc fix for the following issue: https://bugs.openjdk.java.net/browse/JDK-8248184 The proposed patch is here: --- old/src/java.base/share/classes/java/time/temporal/ChronoField.java 2020-06-23 14:43:43.0 -0700 +++

Re: RFR: 8247706: Unintentional use of new Date(year...) with absolute year

Thanks, Martin. +1 When you do the push, please update the copyright year to 2020. Naoto On 6/17/20 12:30 PM, Roger Riggs wrote: Hi Martin, Looks fine, I'm embarrassed by the one in the java.time tests, we should have known better. Thanks, Roger On 6/17/20 3:22 PM, Martin Buchholz

RFR: 8245448: Remove minimum 4 digit requirement from Year.parse()

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8245448 The proposed changeset and its CSR are located at: https://cr.openjdk.java.net/~naoto/8245448/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8246623 Naoto

RFR: 8246721: java/util/Locale/LocaleProvidersRun.java failed on Windows platforms.

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8246721 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8246721/webrev.00/ The test has been modified to deal with double quotes in string arguments consistently across

Re: [15] RFR: 8246662: Test java/time/test/java/time/format/TestUnicodeExtension.java failed on japanese locale.

Hi Joe, On 6/9/20 11:54 AM, Joe Wang wrote: Looks fine to me for the implementation of the method. Thank you for the review! It's interesting to observe that the with methods, as demonstrated in the test, always create a new DateTimeFormatter instance. The Javadoc also said so, e.g.

Re: [15] RFR: 8246662: Test java/time/test/java/time/format/TestUnicodeExtension.java failed on japanese locale.

Thanks! On 6/9/20 11:48 AM, Roger Riggs wrote: Hi Naoto, Looks fine. fyi,  Objects.equals(a,b) can replace DateTimeFormatter: 1510-1511. Right. I will replace it before the push. Naoto Thanks, Roger On 6/9/20 1:01 PM, naoto.s...@oracle.com wrote: Hi Roger, Thanks for the review. On

Re: [15] RFR: 8246662: Test java/time/test/java/time/format/TestUnicodeExtension.java failed on japanese locale.

Hi Roger, Thanks for the review. On 6/9/20 8:52 AM, Roger Riggs wrote: Hi Naoto, Since the default locale is being changed even briefly, the test should be run in /othervm. Add:  @run testng/othervm ... All java/time/test/java/time/format tests are run in othervm mode by default, defined

[15] RFR: 8246662: Test java/time/test/java/time/format/TestUnicodeExtension.java failed on japanese locale.

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8246662 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8246662/webrev.00/ This is a regression caused by the fix to JDK-8244245, where the locale related fields in the

Re: [15] RFR: 8246261: TCKLocalTime.java failed due to "AssertionError: expected [18:14:22] but found [18:14:23]"

Updated webrev: http://cr.openjdk.java.net/~naoto/8246261/webrev.01/ Naoto On 6/1/20 6:30 PM, naoto.s...@oracle.com wrote: Hi Joe, In fact, this bug was possibly revealed by the fix to 8242504, where the system clock precision is now nanoseconds. Before that, it used to be millisecond

Re: [15] RFR: 8246261: TCKLocalTime.java failed due to "AssertionError: expected [18:14:22] but found [18:14:23]"

Hi Joe, In fact, this bug was possibly revealed by the fix to 8242504, where the system clock precision is now nanoseconds. Before that, it used to be millisecond precision, so the first try for the exact match succeeded for most of the cases. Even with the nano precision fix, most of the

[15] RFR: 8246261: TCKLocalTime.java failed due to "AssertionError: expected [18:14:22] but found [18:14:23]"

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8246261 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8246261/webrev.00/ The test case compares two LocalTime objects, created with LocalTime.now(Clock/ZoneId). So

Re: [PATCH] Typo in java.util.regex.Pattern

Looks good! Naoto On 5/30/20 3:40 PM, Lance Andersen wrote: All good Naoto Here is the revised diff — $ hg diff src/java.base/share/classes/java/util/regex/Pattern.java *diff -r 968b57610c0f src/java.base/share/classes/java/util/regex/Pattern.java* *---

Re: [PATCH] Typo in java.util.regex.Pattern

Thanks, Pavel. I think the year stays 2006 then. Sorry, Lance, for the false alarm. Naoto On 5/30/20 2:34 PM, Pavel Rappo wrote: I'm holding a copy in my hands right now. I can see "ISBN: 978-0-596-52812-6" written on the back of the book. Searching that number in the database yields the

Re: [PATCH] Typo in java.util.regex.Pattern

Just noticed that the release year of the 3rd ed. should also be changed to 2009 in the next line. Naoto On 5/30/20 4:02 AM, Lance Andersen wrote: I can sponsor this for you Best Lance -- Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1

Re: 8239013: java.util.logging.Logger catalog cache keeps strong references to ResourceBundles

Looks good to me. Naoto On 5/22/20 2:33 AM, Daniel Fuchs wrote: Hi, Please find below a fix for: 8239013: java.util.logging.Logger catalog cache keeps strong references to ResourceBundles https://bugs.openjdk.java.net/browse/JDK-8239013 webrev:

Re: [15] RFR: 8245241: Incorrect locale provider preference is not logged

Hi Daniel, Thank you for your review! On 5/21/20 3:42 AM, Daniel Fuchs wrote: Hi Naoto, Logging uses: ZonedDateTime zdt = ZonedDateTime.ofInstant(     record.getInstant(), ZoneId.systemDefault()); and then String.format("%1$tb %1$td, %1$tY %1$tl:%1$tM:%1$tS %1$Tp %2$s%n%4$s:

Re: [15] RFR: 8239480: Support for CLDR version 37

Hi Joe, thanks again! The license file "unicode-license.txt" is actually included in the version 37 artifact: https://github.com/unicode-org/cldr/blob/release-37/unicode-license.txt The changeset uses the file as is, so I believe it is OK. Naoto On 5/20/20 6:51 PM, Joe Wang wrote: Looks

Re: [15] RFR: 8245241: Incorrect locale provider preference is not logged

Hi Joe, thanks for the review. On 5/20/20 4:10 PM, Joe Wang wrote: Hi Naoto, I don't seem to see the DateFormat class or the getDateInstance methods specify how errors may be handled (or logged). Is that stated somewhere else? Description of the system property "java.locale.providers" is

[15] RFR: 8239480: Support for CLDR version 37

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8239480 The proposed changeset, and the release note draft are located at: https://cr.openjdk.java.net/~naoto/8239480/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8243581 This updates the

[15] RFR: 8245241: Incorrect locale provider preference is not logged

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8245241 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8245241/webrev.00/ Incorrect user-provided provider preference is supposed to be logged. However it is not so

Re: RFR [15] 8244342: Compilation warnings about unexpected serialization related method signatures.

+1 Naoto On 5/15/20 11:35 AM, Lance Andersen wrote: Looks ok joe On May 15, 2020, at 1:54 PM, Joe Wang wrote: Hi, Please review a fix for the compilation warnings. Thanks Roger for the detailed instructions! If you could verify the fix with the work-in-progress processor, that would be

Re: [15] RFR: 8239383: Support for Unicode 13.0

Thanks for the review, Joe! On 5/13/20 2:58 PM, Joe Wang wrote: +1 They are from the reference files, but it's interesting to note the ranges in Blocks.txt (that Character.java follows) for Tangut Supplement (18D00..18D8F) and CJK Unified Ideographs Extension G (3..3134F) are slightly

Re: [15] RFR: 8239383: Support for Unicode 13.0

Ping. Naoto On 5/7/20 2:59 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8239383 Its CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8239504

Re: RFR: 8244855 : Remove unused "getParent" function from Windows jni_util_md.c

Looks good, with the change to copyright year to "2020." Naoto On 5/12/20 12:12 PM, Lance @ Oracle wrote: +1 Best, Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Sent from my

Re: RFR: 8244767 - Potential non-terminated string in getEncodingInternal() on Windows

+1 Naoto On 5/11/20 5:25 PM, Brian Burkhalter wrote: Hi Brent, It looks OK to me. Brian On May 11, 2020, at 4:36 PM, Brent Christian wrote: Please review this small fix in Windows native code: Bug: https://bugs.openjdk.java.net/browse/JDK-8244767

Re: JDK-8226810: An other case and a small change suggestion

Hi Johannes, On 5/8/20 1:37 PM, Johannes Kuhn wrote: Thanks. I think strcpy(ret+2, "1252") vs. strcpy(ret, "Cp1252")  is a just matter of style. I would prefer the later, as it makes the intent clear. But not my call. I thought the former was clearer, as at that point, Cp/MS/GB part is not

Re: JDK-8226810: An other case and a small change suggestion

Ditto. Good catch! I am not sure the fix would address the issue in 8226810 (cannot confirm it either, as my Windows box is at my office where I cannot enter at the moment :-), but this definitely looks like a bug. I would change the additional line to "strcpy(ret+2, "1252");" as Cp is copied

[15] RFR: 8239383: Support for Unicode 13.0

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8239383 Its CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8239504 https://cr.openjdk.java.net/~naoto/8239383/webrev.00/ This updates java.lang.Character class

Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values

Hi Roger, Thank you for the review. Wrapped the long lines as suggested, along with some typo fixes in the comments: https://cr.openjdk.java.net/~naoto/8244245/webrev.01/ Naoto On 5/7/20 7:43 AM, Roger Riggs wrote: Hi Naoto, Looks fine with a small source edit below.

Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values

Hi Joe, Thank you for the review. The removed check was explicitly avoiding the default chrono/number in the locale overriding the current locale values, which is exactly what this issue is trying to remove. As Stephen wrote in another email, Unicode Extensions are correctly dealt in

[15]: RFR: 8244245: localizedBy() should override localized values with default values

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244245 The CSR and proposed changeset are located at: https://bugs.openjdk.java.net/browse/JDK-8244246 https://cr.openjdk.java.net/~naoto/8244245/webrev.00/ This stems from the closed issue

Re: [15] RFR: 8244459: Optimize the hash map size in LocaleProviderAdapters

Hi Stuart, On 5/5/20 4:29 PM, Stuart Marks wrote: As for Naoto's changeset, I don't think it should be held up while we bikeshed this. :-) I'm ok with whatever he decides. I don't think the number of language tags exceed 2^29, unless languages in the whole universe are counted :-) So I would

Re: [15] RFR: 8244459: Optimize the hash map size in LocaleProviderAdapters

Thanks, all. I didn't see this coming! If I understand the discussion correctly, Peter's suggestion is the most optimal (Mark, your formula produces 1 for the expected size is 0, although it won't be happening in this particular case). And Joe, thank you for finding my silly mistake :-) So

[15] RFR: 8244459: Optimize the hash map size in LocaleProviderAdapters

And here is the fix. Please review. http://cr.openjdk.java.net/~naoto/8244459/webrev.00/ Naoto On 5/5/20 10:25 AM, naoto.s...@oracle.com wrote: Hi Peter, You are correct. Thanks. I'll remove that initial value of 16. Naoto On 5/5/20 9:37 AM, Peter Levart wrote: Hi Naoto, On 4/30/20 12:18

Re: [15] RFR: 8244152: Remove unnecessary hash map resize in LocaleProviderAdapters

Hi Peter, You are correct. Thanks. I'll remove that initial value of 16. Naoto On 5/5/20 9:37 AM, Peter Levart wrote: Hi Naoto, On 4/30/20 12:18 AM, naoto.s...@oracle.com wrote: Hello, Please review this small fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244152

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports - (core libraries)

Hi Mikael, I took a look at i18n related files. It looks good overall. One nit in java/nio/charset/Charset/DefaultCharsetTest.java: If the test is only applicable to linux (@requires os.family == "linux" in jtreg tag after the change), the condition isLinux() is no longer needed at line 73.

[15] RFR: 8244152: Remove unnecessary hash map resize in LocaleProviderAdapters

Hello, Please review this small fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8244152 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8244152/webrev.00/ The hash map used there didn't have initial capacity, even though the exact numbers are

Re: RFR: 8242541: Small charset issues (ISO8859-16, x-eucJP-Open, x-IBM834 and x-IBM949C)

Looks good. Naoto On 4/28/20 11:41 AM, Ichiroh Takiguchi wrote: Hello Naoto. Thank you for your attention. And I'm sorry about bad response. I added ISO8859_16 entry into test/jdk/java/nio/charset/Charset/RegisteredCharsets.java Could you review the fix again ? Bug:   

Re: RFR [15] 8243541: (tz) Upgrade time-zone data to tzdata2020a

Looks OK to me. Thanks for the update. Naoto On 4/27/20 11:19 AM, Kiran Ravikumar wrote: Hi Martin and Andrew, Thanks for the review and providing a direction towards updating the translations. Here is an updated webrev with the changes:

[15] RFR: 8243664: JavaDoc of CompactNumberFormat points to wrong enum

Hello, Please review this small doc fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8243664 Here is the diff: --- --- old/src/java.base/share/classes/java/text/CompactNumberFormat.java 2020-04-27 09:09:14.0 -0700 +++

Re: RFR: 8243254: Examine ZipFile slash optimization for non-ASCII compatible charsets

+1 Naoto On 4/22/20 9:13 AM, Lance Andersen wrote: Hi Claes, The latest version looks good. Thank you for the patch. Best Lance On Apr 22, 2020, at 6:26 AM, Claes Redestad wrote: Hi, new webrev based on discussions here and offline:

Re: RFR: 8242541: Small charset issues (ISO8859-16, x-eucJP-Open, x-IBM834 and x-IBM949C)

Hi Takiguchi-san, Change looks good. I'd expect a test case in open/test/jdk/java/nio/charset/Charset/RegisteredCharsets.java for the added "ISO8859_16" alias. Naoto On 4/20/20 6:37 PM, Ichiroh Takiguchi wrote: Hello. Could you review the fix ? Bug:   

Re: RFR: 8241055: Regex Grapheme Matcher Performance Depends too much on Total Input Sequence Size

Looks good. As you noticed, the error does not seem to occur with JDK14 as bootjdk. I will push it after I confirmed OK on my side. Thank you for your contribution. Naoto On 4/16/20 12:26 PM, Philipp Kunz wrote: Hi Naoto, Changed as you suggested. However, I could not verify it due to,

Re: RFR: 8241055: Regex Grapheme Matcher Performance Depends too much on Total Input Sequence Size

Hi Philipp, Thanks for the update. I like you enriched the RegExTest.java. Two nits: - RegExTest.java at line 4805: I'd use parentheses for this return statement. - PatternBench.java should keep the original copyright year, i.e., "2020," -> "2019, 2020," Otherwise, it looks good. Naoto

Re: RFR: 8242283: Can't start JVM when java home path includes non-ASCII character

Suenaga-san, Looks good to me. Please add noreg-hard if you are not planning to provide test cases. Thank you for fixing this issue. Naoto On 4/10/20 7:29 AM, Yasumasa Suenaga wrote: Hi all, Please review this change:   JBS: https://bugs.openjdk.java.net/browse/JDK-8242283   webrev:

Re: RFR [15/java.xml] 8242470 : Upgrade Xerces to Version 2.12.1

+1 Naoto On 4/9/20 2:28 PM, Joe Wang wrote: Please review an update to Xerces 2.12.1. Xerces 2.12.1 was a bug fix release. Bugs in the release were either already in the JDK or not applicable. This patch therefore includes only an update to the md file. ---

Re: RFR [15/java.xml] 8237187 Obsolete references to java.sun.com

++1 Naoto On 4/8/20 4:34 PM, Lance Andersen wrote: +1 On Apr 8, 2020, at 7:28 PM, Joe Wang wrote: Please review a doc-only change to replace links to sun.com with ones to oracle.com. Only material changes were the two sun.com links, others format, e.g. moving the anchor brackets to

[15] RFR (XXS): 8242337: javadoc typo in NumberFormat::setMinimumFractionDigits

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8242337 Here is the proposed diff: --- diff -r 706df347bcc2 src/java.base/share/classes/java/text/NumberFormat.java --- a/src/java.base/share/classes/java/text/NumberFormat.java +++

Re: [15] RFR: 8242010: Upgrade IANA Language Subtag Registry to Version 2020-04-01

Thanks Roger. I will fix the typo before the push. Naoto On 4/7/20 12:47 PM, Roger Riggs wrote: Hi Naoto, Looks fine. In EquivMapsGenerator.java. typo: grandfahered -> grandfathered Thanks, Roger On 4/7/20 1:52 PM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the

[15] RFR: 8242010: Upgrade IANA Language Subtag Registry to Version 2020-04-01

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8242010 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8242010/webrev.00/ Besides the upgrade of the data file, I fixed the issue where EquivMapsGenerator had been

Re: RFR [15/java.xml] 8238183: SAX2StAXStreamWriter cannot deal with comments prior to the root element

Looks good. Thanks for the update. Naoto On 3/30/20 3:44 PM, Joe Wang wrote: On 3/30/20 3:12 PM, naoto.s...@oracle.com wrote: Hi Joe, Much better and cleaner! One nit is that you could remove "docLocator != null &&" as instanceof checks null. Indeed, null check is removed:

Re: RFR [15/java.xml] 8238183: SAX2StAXStreamWriter cannot deal with comments prior to the root element

Hi Joe, Much better and cleaner! One nit is that you could remove "docLocator != null &&" as instanceof checks null. Naoto On 3/30/20 2:56 PM, Joe Wang wrote: Hi Naoto, Thanks for the review! I refactored the code around getting the XML version and encoding from the locator to get rid of

Re: 6202130: Need to handle UTF-8 values and break up lines longer than 72 bytes

Hi Philipp, Sorry for the delay. On 3/25/20 11:52 AM, Philipp Kunz wrote: Hi Naoto, See another patch attached with Locale.ROOT for the BreakIterator. I will be glad to hear of any feedback. I wonder how your change affects the performance, as it will do String.getBytes(UTF-8) per each

Re: RFR [15/java.xml] 8238183: SAX2StAXStreamWriter cannot deal with comments prior to the root element

Hi Joe, Can writeStartDocument() be simplified by checking "docLocator instanceof Locator2"? This way it won't need to catch CCE and issue no-arg version. Otherwise looks good to me. Naoto On 3/30/20 11:02 AM, Joe Wang wrote: Hi, Please review a fix for the StAXResult impl. The issue was

Re: RFR: 8241055: Regex Grapheme Matcher Performance Depends too much on Total Input Sequence Size

Hi Philipp, I looked at the patch, and it looks good to me. As to the test case, since it is measuring the performance, I would make it in a JMH test. Maybe you would want to put it similar to open/test/micro/org/openjdk/bench/java/util/regex/PatternBench.java Minor suggestion: I would

Re: [15] RFR: 8241311: Move some charset mapping tests from closed to open

Hi Amy, I ended up the fix pretty much as of ver 06, as those data file formats are different. It'd require some amount of refactoring to those other two tests, and CoderTest.java is a dependence to ConverterTest.java, so I left them as-is. One minor change from 06 is to add @modules

Re: [15] RFR: 8241311: Move some charset mapping tests from closed to open

On 3/24/20 9:24 AM, Ichiroh Takiguchi wrote: Hello Naoto. I tested webrev.06 code. It's fine, thanks. I'm interested in about @module for these testcases. I think webrev.04 code worked via jtreg. I could not see any warning. At this case, @module is required ? Yes. The tag lets jtreg not run

Re: [15] RFR: 8241311: Move some charset mapping tests from closed to open

Thanks again. I will look into it and come up with better migration of those tests. Naoto On 3/23/20 9:45 PM, Amy Lu wrote: Thank you Naoto for the quick update. Just more findings ...(sorry for not sending earlier) CoderTest.java Looks like this test has already been migrated (with

Re: [15] RFR: 8241311: Move some charset mapping tests from closed to open

Hi Amy, Good point. Updated. https://cr.openjdk.java.net/~naoto/8241311/webrev.06/ Naoto On 3/23/20 7:43 PM, Amy Lu wrote: Hi, Naoto CoderTest.java TestConv.java Should they also include @modules jdk.charsets ? Thanks, Amy On 3/21/20 12:21 AM, naoto.s...@oracle.com wrote: Hello, Please

Re: [15] RFR: 8241311: Move some charset mapping tests from closed to open

Hi Takiguchi-san, On 3/23/20 5:48 AM, Ichiroh Takiguchi wrote: Hello Naoto. I'm not reviewer, but I have a concern about following code on test/jdk/sun/nio/cs/mapping/TestConv.java ==  98 } catch (Exception ex) {  99 System.out.println("Exception thrown while

Re: RFR: JDK-8241463 Move build tools to respective modules

Hi Magnus, I looked at i18n related changes: make/CopyInterimTZDB.gmk make/ToolsJdk.gmk make/gendata/Gendata-java.base.gmk make/gendata/GendataBreakIterator.gmk make/gendata/GendataTZDB.gmk make/gensrc/GensrcCharacterData.gmk make/gensrc/GensrcEmojiData.gmk They look ok to me. The *.java

Re: RFR: 8241055: Regex Grapheme Matcher Performance Depends too much on Total Input Sequence Size

I am planning to work on it. Naoto On 3/21/20 7:58 AM, Philipp Kunz wrote: Hi, Any opinions on the attached patch or someone tempted to sponsor it? Regards, Philipp

Re: 6202130: Need to handle UTF-8 values and break up lines longer than 72 bytes

Hi Philipp, Right, Locale.ROOT is more appropriate here by definition, though the implementation is the same. Naoto On 3/21/20 5:19 AM, Philipp Kunz wrote: Hi Naoto and everyone, There are almost as many occurrences of Locale.ROOT as Locale.US which made me wonder which one is more

[15] RFR: 8241311: Move some charset mapping tests from closed to open

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8241311 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8241311/webrev.04/ This is simply to move some test cases that have been in closed repository into open repository.

Re: [15] RFR: 8241082: Upgrade IANA Language Subtag Registry data to 03-16-2020 version

Hi Roger, thanks for the review. On 3/18/20 7:42 AM, Roger Riggs wrote: Hi Naoto, EquivMapsGenerator.java: 242 It looks odd to put the warning about being an auto-generated file in the middle of the declarations. Perhaps add it to the headerText. The existing maps are not pre-sized, is it

[15] RFR: 8241082: Upgrade IANA Language Subtag Registry data to 03-16-2020 version

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8241082 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8241082/webrev.00/ It is simply updating the data file. Since there is no change in equivalency of language tags, no

Re: [15] RFR: 8240626: Some of the java.time.chrono.Eras return empty display name for some styles and locales

Hi Joe, Thank you for the review. Since those names are filled at the JDK build time, there is no way to confirm the localized ones are from the locale itself or its parents, unless parsing CLDR's source XML files in the test at the runtime. I think it is enough to just ensure there's no

[15] RFR: 8240626: Some of the java.time.chrono.Eras return empty display name for some styles and locales

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8240626 The proposed chageset is located at: https://cr.openjdk.java.net/~naoto/8240626/webrev.00/ In some locales, CLDR only provides partial translations of era names, e.g., only HEISEI and REIWA

Re: RFR [15/java.xml] 8240982 Incorrect copyright header in BCEL 6.4.1 sources

Looks good, Joe. Naoto On 3/13/20 10:32 AM, Joe Wang wrote: Please review a quick fix for the missing commas after the 2nd year in the copyright header: diff --git a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/Const.java

Re: [15] RFR: 8216332: Grapheme regex does not work with emoji sequences

Ping. Any takers? Naoto On 3/5/20 8:58 AM, naoto.s...@oracle.com wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8216332 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8216332/webrev.00/ Before this fix, the

Re: 6202130: Need to handle UTF-8 values and break up lines longer than 72 bytes

Hi Phillip, Sure. Would you please file Grapheme related issues at bugs.java.com? This is the route for whom doesn't have JBS id to file bugs. Naoto On 3/10/20 11:27 PM, Philipp Kunz wrote: Hi Naoto, Thank you for your reply. I fully agree to your reluctance or inhibition to change the

Re: RFR: 8240725: Some functions might not work with CJK character

Looks good to me. Thanks for the fix! Naoto On 3/10/20 5:39 PM, Yasumasa Suenaga wrote: Hi Sato-san, Thanks for your comment. I uploaded new webrev:   http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.03/ On 2020/03/11 2:08, naoto.s...@oracle.com wrote: Hi Suenaga-san, Thank you

Re: RFR: 8240725: Some functions might not work with CJK character

Ah, thanks Martin. Never mind about the comment wrt free(NULL) then. Naoto On 3/10/20 11:30 AM, Martin Buchholz wrote: On Tue, Mar 10, 2020 at 10:08 AM > wrote: Then I noticed (should have noticed in the first place, sorry) that the error handling in

Re: 6202130: Need to handle UTF-8 values and break up lines longer than 72 bytes

Hi Philipp, The reason that the current implementation of Grapheme boundary check does it is that, some rules require information before the boundary, i.e. the rule GB12 in Unicode Segmentation [1] reads: "do not break between regional indicator (RI) symbols if there is an odd number of RI

Re: RFR: 8240725: Some functions might not work with CJK character

Hi Suenaga-san, Thank you for the update. I think your changes to the return values of MultiByteToWideChar look good. Then I noticed (should have noticed in the first place, sorry) that the error handling in canonicalize_md.c is incorrect (unrelated to your change). All error cases "goto

Re: RFR 8232161: Align some one-way conversion in MS950 charset with Windows

Looks good to me. Thanks for the update. Naoto On 3/9/20 10:37 AM, Ichiroh Takiguchi wrote: Hello Naoto. I appreciate your comments. I modified TestMS950.java testcase. I think it's easy to read. Could you review the fix again ? Bug:    https://bugs.openjdk.java.net/browse/JDK-8232161

Re: RFR: 8240725: Some functions might not work with CJK character

Hi Suenaga-san, I think the return value from the second MultiByteToWideChar invocation should be examined (non-zero), and if it was not successful, appropriate action should be taken. Naoto On 3/9/20 3:50 AM, Yasumasa Suenaga wrote: Hi all, Please review this change:   JBS:

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

Thanks, Stephen. Updated the webrev for those two suggestions: http://cr.openjdk.java.net/~naoto/8239836/webrev.04/ Naoto On 3/8/20 4:22 PM, Stephen Colebourne wrote: standardOffsets[0] == wallOffsets[0] needs to use .equals() Unrelated, there is a Javadoc/spec error at line 578 of the

Re: RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

Looks good. Thanks for the update. Naoto On 3/6/20 8:17 AM, Roger Riggs wrote: Hi Naoto, Updated webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html I don't think getting handle count of your own process can ever fail. But in the abundance of caution, added a check

Re: RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

Hi Roger, In the test, the error value (-1) from the native code seems silently suppressed. Should it be caught/reported in the java side? nit: copyright year in ProcessImpl.java -> 2020. Naoto On 3/5/20 12:51 PM, Roger Riggs wrote: Please review a change to the Windows ProcessImpl to

[15] RFR: 8216332: Grapheme regex does not work with emoji sequences

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8216332 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8216332/webrev.00/ Before this fix, the rule GB11 (Do not break within emoji modifier sequences or emoji zwj

Re: RFR 8232161: Align some one-way conversion in MS950 charset with Windows

On 3/4/20 9:18 AM, Ichiroh Takiguchi wrote: Hello Naoto. I appreciate your comments. I applied following changes: * MS950.nr and TestMS950.java data were sorted by Unicode order * Added some comments into TestMS950.java * Change comment on MS950.map Could you review the fix ? Bug:   

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

Hi Stephen, Thank you for the detailed explanation and correcting the implementation. I incorporated all the suggestions below, as well as adding a new test for isFixedOffset() implementation (with some clean-up): https://cr.openjdk.java.net/~naoto/8239836/webrev.03/ Please take a look at

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

Thanks, Joe. Updated: http://cr.openjdk.java.net/~naoto/8239836/webrev.02/ Naoto On 3/3/20 8:59 AM, Joe Wang wrote: On 3/3/20 8:27 AM, naoto.s...@oracle.com wrote: Hi Joe, thanks for the review. Are you suggesting something like isFixedOffset() {     return isFixedOffset; } Yes,

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

Hi Joe, thanks for the review. Are you suggesting something like isFixedOffset() { return isFixedOffset; } Naoto On 3/2/20 11:20 PM, Joe Wang wrote: Hi Naoto, The fix would be fine if you want to keep it as is since it does work. I noticed though that for standard zones (the ones

Re: RFR 8232161: Align some one-way conversion in MS950 charset with Windows

Hi Takiguchi-san, A few comments: - I'd recommend sorting the entries in MS950.nr and test data in TestMS950.java for readability. - Add some comment about the objective in the test. It'd be hard for engineers who have no previous knowledge to these bytes. Naoto On 3/2/20 9:33 AM,

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

Hi Roger, thanks for the review. On 3/2/20 8:44 AM, Roger Riggs wrote: Hi Naoto, look ok. ZoneRules.java: 488, 644, 761, 791 I'd suggest calling isFixedOffset() instead of repeating the code: standardTransitions.length == 0 && savingsInstantTransitions.length == 0 Modified as suggested:

[15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8239836 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8239836/webrev.00/ It turned out that 'transitionList' is not necessarily a superset of

Re: RFR: 8239965: XMLEncoder/Test4625418.java fails due to "Error: Cp943 - can't read properly"

Looks good. Naoto On 2/27/20 6:00 AM, Ichiroh Takiguchi wrote: Hello Naoto. I appreciate your comments. And sorry for my easy mistake. Could you review the fix again ? Bug:    https://bugs.openjdk.java.net/browse/JDK-8239965 Change: https://cr.openjdk.java.net/~itakiguchi/8239965/webrev.01/

Re: RFR: 8239965: XMLEncoder/Test4625418.java fails due to "Error: Cp943 - can't read properly"

Takiguchi-san, Some nits: - Please add 'noreg-self' to the jira issue. - Copyright year should be modified. - Add the bug number to '@bug' tag. Naoto On 2/26/20 10:03 AM, Ichiroh Takiguchi wrote: Hello. Could you review the fix ? Bug:    https://bugs.openjdk.java.net/browse/JDK-8239965

<    5   6   7   8   9   10   11   12   13   14   >