Re: RFR: 8266155: Convert java.base to use Stream.toList()
On Tue, 27 Apr 2021 21:34:02 GMT, Ian Graves wrote: > 8266155: Convert java.base to use Stream.toList() Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3734
Re: RFR: 8266014: Regression brought by optimization done with JDK-4926314
On Tue, 27 Apr 2021 01:26:53 GMT, Brian Burkhalter wrote: > Please consider this request to correct a minor problem with the optimization > added for JDK-4926314. The change is to attempt to read the number of > elements remaining in the target buffer unless that number is non-positive in > which case read zero. The test addition fails without and passes with the > implementation change. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3708
Re: RFR: JDK-8265496: improve null check in DeflaterOutputStream/InflaterInputStream [v2]
On Mon, 26 Apr 2021 02:36:54 GMT, Hamlin Li wrote:
>> code like below will create Deflater before null check, although it's not a
>> real mem leak, but it's better to do null check before new Deflater.
>>
>> try {
>> DeflaterOutputStream dos = new DeflaterOutputStream(null);
>> } catch (NullPointerException e) {
>> passed = true;
>> }
>> Similar issues exist in several other classes.
>
> Hamlin Li has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update copyrights.
Marked as reviewed by naoto (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/3681
Integrated: 8264208: Console charset API
On Fri, 9 Apr 2021 16:47:55 GMT, Naoto Sato wrote: > Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. This pull request has now been integrated. Changeset: bebfae48 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/bebfae48 Stats: 214 lines in 10 files changed: 184 ins; 12 del; 18 mod 8264208: Console charset API Reviewed-by: joehw, rriggs, alanb - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8265746: Update java.time to use instanceof pattern variable (part II) [v2]
On Fri, 23 Apr 2021 15:38:53 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Removed redundant braces > - Merge remote-tracking branch 'origin/master' into JDK-8265746 > - 8265746: Update java.time to use instanceof pattern variable (part II) Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3650
Re: RFR: 8264208: Console charset API [v11]
On Thu, 22 Apr 2021 16:29:35 GMT, Roger Riggs wrote: >> Then `charset()` in the shared secret would return `null`. Would that >> suffice your case? > > I read lines 575-587 as initializing CHARSET regardless of whether the > Console was created. OK, revived the charset() method. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v12]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Revived charset() in JavaIOAccess interface. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/e585d16f..2d90f8df Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=10-11 Stats: 5 lines in 2 files changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v11]
On Thu, 22 Apr 2021 16:03:12 GMT, Roger Riggs wrote: >> Would the singleton `Console.cons` be instantiated in your use case? It is >> created only when isatty() (or Windows' equivalent) in the native code >> returns true. > > Not always, for example, if stderr was redirected to a terminal but not stdin > and stdout. > The istty check is only true if both stdin and stdout are ttys. Then `charset()` in the shared secret would return `null`. Would that suffice your case? - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v11]
On Thu, 22 Apr 2021 15:18:11 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 13 additional >> commits since the last revision: >> >> - Refined the test case. >> - Merge branch 'master' into JDK-8264208 >> - Changed shell based test into java based >> - Added link to Charset#defaultChaset() in InputStreamReader. >> - Modified javadocs per suggestions. >> - Added @see links. >> - Added Console::charset() relation with System.in >> - Added comment to System.out/err init. >> - Reflected further review comments. >> - Reverted PrintStream changes >> - ... and 3 more: >> https://git.openjdk.java.net/jdk/compare/51627555...e585d16f > > src/java.base/share/classes/java/io/Console.java line 597: > >> 595: return null; >> 596: } >> 597: }); > > Please keep the charset() method and return CHARSET. > > I'm looking at a use case that needs to know the platform charset regardless > of whether the console exists. > When a process is launched it may be redirected to /dev/tty or a pseudo tty > and in that case > a Reader from that stream should be able to use the encoding of the platform. > Its still a work in progress, but it would save some refactoring or > duplication later. Would the singleton `Console.cons` be instantiated in your use case? It is created only when isatty() (or Windows' equivalent) in the native code returns true. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v7]
On Wed, 21 Apr 2021 11:06:16 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Updated single letter pattern variable name in java/time/Duration > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Updated single letter pattern variable names > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - Merge remote-tracking branch 'origin/master' into JDK-8263668 > - 8263668: Update java.time to use instanceof pattern variable Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8264208: Console charset API [v11]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision: - Refined the test case. - Merge branch 'master' into JDK-8264208 - Changed shell based test into java based - Added link to Charset#defaultChaset() in InputStreamReader. - Modified javadocs per suggestions. - Added @see links. - Added Console::charset() relation with System.in - Added comment to System.out/err init. - Reflected further review comments. - Reverted PrintStream changes - ... and 3 more: https://git.openjdk.java.net/jdk/compare/8cc07521...e585d16f - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/238dbb42..e585d16f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=09-10 Stats: 72701 lines in 1955 files changed: 33448 ins; 34099 del; 5154 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v6]
On Tue, 20 Apr 2021 17:46:38 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > Updated single letter pattern variable names Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8264208: Console charset API [v10]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with two additional commits since the last revision: - Changed shell based test into java based - Added link to Charset#defaultChaset() in InputStreamReader. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/083f6180..238dbb42 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=08-09 Stats: 88 lines in 3 files changed: 27 ins; 51 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Integrated: 8265375: Bootcycle builds fail with StackOverflowError in cldrconverter
On Fri, 16 Apr 2021 21:10:42 GMT, Naoto Sato wrote: > Please review the fix to the tier4 build failure. The piece of code that made > into `CLDRLocaleProviderAdapter.java` was also needed in the build tool > counterpart (`CLDRConverter`). This pull request has now been integrated. Changeset: ff499701 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/ff499701 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod 8265375: Bootcycle builds fail with StackOverflowError in cldrconverter Reviewed-by: joehw - PR: https://git.openjdk.java.net/jdk/pull/3553
Re: RFR: 8265375: Bootcycle builds fail with StackOverflowError in cldrconverter [v2]
> Please review the fix to the tier4 build failure. The piece of code that made > into `CLDRLocaleProviderAdapter.java` was also needed in the build tool > counterpart (`CLDRConverter`). Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Copyright year update. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3553/files - new: https://git.openjdk.java.net/jdk/pull/3553/files/91326786..e54cb6ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3553=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3553=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3553.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3553/head:pull/3553 PR: https://git.openjdk.java.net/jdk/pull/3553
RFR: 8265375: Bootcycle builds fail with StackOverflowError in cldrconverter
Please review the fix to the tier4 build failure. The piece of code that made into `CLDRLocaleProviderAdapter.java` was also needed in the build tool counterpart (`CLDRConverter`). - Commit messages: - 8265375: Bootcycle builds fail with StackOverflowError in cldrconverter Changes: https://git.openjdk.java.net/jdk/pull/3553/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3553=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265375 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3553.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3553/head:pull/3553 PR: https://git.openjdk.java.net/jdk/pull/3553
Re: RFR: 8264208: Console charset API [v9]
On Fri, 16 Apr 2021 18:15:41 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Modified javadocs per suggestions. > > src/java.base/share/classes/java/io/InputStreamReader.java line 48: > >> 46: * For top efficiency, consider wrapping an InputStreamReader within >> a >> 47: * BufferedReader. For example: >> 48: * > > Oddly, none of the reference in this class to the default charset are links > to Charset.defaultCharset(). > That would be a useful addition, either in the class javadoc or in the 1-arg > constructor that uses the default charset. Thanks, Roger. Both are good suggestions. Will incorporate them into the next iteration. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v5]
On Fri, 16 Apr 2021 04:06:54 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed fix for >> https://bugs.openjdk.java.net/browse/JDK-8262108? >> >> As noted in a comment in that issue, the bug relates to the return value of >> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The >> implementation has started returning invalid values for the `AM_PM` field >> after the "day period" support was added recently in the JDK as part of >> https://bugs.openjdk.java.net/browse/JDK-8262108. >> >> The commit here adds a check in the internal implementation of the display >> name handling logic, to special case the `AM_PM` field and properly convert >> the display name array indexes (which is an internal detail) to valid values >> that represent the `AM_PM` calendar field. >> >> The commit also has a new jtreg test case `CalendarDisplayNamesTest` >> reproduces this issue and verifies the fix. >> >> After this fix was introduced, I ran the test in >> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing >> test case `NarrowNamesTest`. Looking at that test case, IMO, the current >> testing in that `NarrowNamesTest` is incorrect and is probably what hid this >> issue in the first place? To fix this, I have added an additional commit >> which updates this test case to properly test the `AM_PM` field values. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Update existing test based on review comments Looks good. Thank you for fixing the issue. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v4]
On Fri, 16 Apr 2021 02:35:23 GMT, Jaikiran Pai wrote:
>> test/jdk/java/util/Calendar/NarrowNamesTest.java line 115:
>>
>>> 113: } else {
>>> 114: testMap(US, AM_PM, ALL_STYLES,
>>> 115: "AM", "PM",
>>
>> What I meant was there is no need to check the providers and introduce the
>> am/pm specific test method. Both `CLDR` and `COMPAT` (i.e., no "if"
>> statement) should be tested with `testMap()` method as other tests do.
>
> Hello Naoto,
>
> As far as I can see, the testMap cannot be used to test the day period
> strings. More specifically, consider this change (git diff) that I did as you
> suggested (unless I misunderstood what you meant):
>
>
> +testMap(US, AM_PM, ALL_STYLES,
> +"AM",
> +"PM",
> +"midnight",
> +"noon",
> +"in the morning",
> +"",
> +"in the afternoon",
> +"",
> +"in the evening",
> +"",
> +"at night",
> +"",
> +RESET_INDEX,
> +"a", "p", "mi", "n", "", "", "", "", "", "", "", "");
> +testMap(US, AM_PM, ALL_STYLES,
> +"AM", "PM",
> +RESET_INDEX,
> +"a", "p");
>
>
>
> This will fail with the following error.
>
> testMap: locale=en_US, field=9, style=0, expected={in the afternoon=6, in the
> evening=8, in the morning=4, at night=10, midnight=2, noon=3, AM=0, PM=1,
> mi=2, a=0, n=3, p=1}, got={AM=0, PM=1, a=0, p=1}
> java.lang.RuntimeException: test failed
>
>
> That failure is due to the `testMap` method expecting these day period string
> to be part of the returned Map from `Calendar.getDisplayNames()` which won't
> be the case because that API (as per the javadoc) will only return a Map
> whose display names have valid field values, so in this case only those
> display names which will have `AM` or `PM` as a value.
>
> If it's easier to review, I will go ahead and push the suggested change in
> this testcase and let if fail so that you can get a chance to look at the
> actual test code and error. Let me know.
Sorry if I wasn't clear. The whole test for am/pm (line 98 - 118) can simply be:
testMap(US, AM_PM, ALL_STYLES,
"AM", "PM",
RESET_INDEX,
"a", "p");
Since we now know that day periods strings won't be leaked into display names,
then the map simply should contain only 4 entries (`AM`, `PM`, `a`, and `p`).
-
PR: https://git.openjdk.java.net/jdk/pull/3463
Integrated: 8258794: Support for CLDR version 39
On Wed, 14 Apr 2021 21:13:51 GMT, Naoto Sato wrote: > Please review the changes to support CLDR version 39. The vast majority of > the changes are purely data changes from Unicode. The only change affected in > logic was in `CLDRLocaleProviderAdapter.java`, where it needed to deal with > CLDR's Norwegian language code switch > (https://unicode-org.atlassian.net/browse/CLDR-2698) This pull request has now been integrated. Changeset: f6e54f2f Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/f6e54f2f Stats: 26326 lines in 815 files changed: 761 ins; 23140 del; 2425 mod 8258794: Support for CLDR version 39 Reviewed-by: joehw, erikj - PR: https://git.openjdk.java.net/jdk/pull/3502
Re: RFR: 8264208: Console charset API [v8]
On Thu, 15 Apr 2021 14:17:11 GMT, Alan Bateman wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added @see links.
>
> src/java.base/share/classes/java/io/Console.java line 397:
>
>> 395: /**
>> 396: * Returns the {@link java.nio.charset.Charset Charset} object used
>> in
>> 397: * the {@code Console}.
>
> What would you think about re-phrasing the first sentence to use "for the
> Console" rather than "in the Console".
Changed to "for the Console", as well as `@return`.
> src/java.base/share/classes/java/lang/System.java line 123:
>
>> 121: *
>> 122: * @see Console#charset()
>> 123: * @see Console#reader()
>
> What would you think about changing the example in InputStreamReader class
> description as part of this?
Replaced `System.in` with generic `anInputStream`, as changing `new
InputStreamReader` with `Console.reader()` would defy the purpose of the
example.
-
PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v9]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Modified javadocs per suggestions. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/5988f600..083f6180 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=07-08 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v4]
On Thu, 15 Apr 2021 01:57:01 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this proposed fix for
>> https://bugs.openjdk.java.net/browse/JDK-8262108?
>>
>> As noted in a comment in that issue, the bug relates to the return value of
>> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The
>> implementation has started returning invalid values for the `AM_PM` field
>> after the "day period" support was added recently in the JDK as part of
>> https://bugs.openjdk.java.net/browse/JDK-8262108.
>>
>> The commit here adds a check in the internal implementation of the display
>> name handling logic, to special case the `AM_PM` field and properly convert
>> the display name array indexes (which is an internal detail) to valid values
>> that represent the `AM_PM` calendar field.
>>
>> The commit also has a new jtreg test case `CalendarDisplayNamesTest`
>> reproduces this issue and verifies the fix.
>>
>> After this fix was introduced, I ran the test in
>> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing
>> test case `NarrowNamesTest`. Looking at that test case, IMO, the current
>> testing in that `NarrowNamesTest` is incorrect and is probably what hid this
>> issue in the first place? To fix this, I have added an additional commit
>> which updates this test case to properly test the `AM_PM` field values.
>
> Jaikiran Pai has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - update existing testcase based on review comment
> - Improve code comment to be clear it's only applicable for
> java.util.Calendar
> - Remove irrelevant setLenient from new testcase
Changes requested by naoto (Reviewer).
test/jdk/java/util/Calendar/NarrowNamesTest.java line 115:
> 113: } else {
> 114: testMap(US, AM_PM, ALL_STYLES,
> 115: "AM", "PM",
What I meant was there is no need to check the providers and introduce the
am/pm specific test method. Both `CLDR` and `COMPAT` (i.e., no "if" statement)
should be tested with `testMap()` method as other tests do.
-
PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8258794: Support for CLDR version 39
On Wed, 14 Apr 2021 21:13:51 GMT, Naoto Sato wrote:
> Please review the changes to support CLDR version 39. The vast majority of
> the changes are purely data changes from Unicode. The only change affected in
> logic was in `CLDRLocaleProviderAdapter.java`, where it needed to deal with
> CLDR's Norwegian language code switch
> (https://unicode-org.atlassian.net/browse/CLDR-2698)
Thanks, Joe.
> Naoto, are you testing GitHub ('s ability to handle a large number of files)
> ;-)
CLDR itself has been hosted on GitHub too, so it shouldn't be a problem
-
PR: https://git.openjdk.java.net/jdk/pull/3502
RFR: 8258794: Support for CLDR version 39
Please review the changes to support CLDR version 39. The vast majority of the changes are purely data changes from Unicode. The only change affected in logic was in `CLDRLocaleProviderAdapter.java`, where it needed to deal with CLDR's Norwegian language code switch (https://unicode-org.atlassian.net/browse/CLDR-2698) - Commit messages: - 8258794: Support for CLDR version 39 - CLDR 38.1 Changes: https://git.openjdk.java.net/jdk/pull/3502/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3502=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258794 Stats: 26326 lines in 815 files changed: 761 ins; 23140 del; 2425 mod Patch: https://git.openjdk.java.net/jdk/pull/3502.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3502/head:pull/3502 PR: https://git.openjdk.java.net/jdk/pull/3502
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v3]
On Wed, 14 Apr 2021 17:14:55 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this proposed fix for
>> https://bugs.openjdk.java.net/browse/JDK-8262108?
>>
>> As noted in a comment in that issue, the bug relates to the return value of
>> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The
>> implementation has started returning invalid values for the `AM_PM` field
>> after the "day period" support was added recently in the JDK as part of
>> https://bugs.openjdk.java.net/browse/JDK-8262108.
>>
>> The commit here adds a check in the internal implementation of the display
>> name handling logic, to special case the `AM_PM` field and properly convert
>> the display name array indexes (which is an internal detail) to valid values
>> that represent the `AM_PM` calendar field.
>>
>> The commit also has a new jtreg test case `CalendarDisplayNamesTest`
>> reproduces this issue and verifies the fix.
>>
>> After this fix was introduced, I ran the test in
>> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing
>> test case `NarrowNamesTest`. Looking at that test case, IMO, the current
>> testing in that `NarrowNamesTest` is incorrect and is probably what hid this
>> issue in the first place? To fix this, I have added an additional commit
>> which updates this test case to properly test the `AM_PM` field values.
>
> Jaikiran Pai has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Update existing NarrowNamesTest to match expectations
> - Naoto's review suggestion
Changes requested by naoto (Reviewer).
src/java.base/share/classes/sun/util/locale/provider/CalendarNameProviderImpl.java
line 193:
> 191: }
> 192: if (field == AM_PM && !javatime && i > PM) {
> 193: // when dealing with calendar fields,
> don't set AM_PM field value
Make it explicit that this block only serves for `java.util.Calendar`, not
`java.time.format.DateTimeFormatter(Builder)`.
test/jdk/java/util/Calendar/CalendarDisplayNamesTest.java line 53:
> 51: for (final int style : styles) {
> 52: final Calendar cal = Calendar.getInstance();
> 53: cal.setLenient(false);
Don't think leniency is relevant here.
test/jdk/java/util/Calendar/NarrowNamesTest.java line 119:
> 117: expectedFieldValues.put("a", Calendar.AM);
> 118: expectedFieldValues.put("p", Calendar.PM);
> 119: testAM_PM(US, ALL_STYLES, expectedFieldValues);
I believe this can be reverted to the original, as the original code compares
the exact map (including the length of the map). , i.e.,
testMap(US, AM_PM, ALL_STYLES,
"AM", "PM",
RESET_INDEX,
"a", "p");
-
PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8264208: Console charset API [v8]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added @see links. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/45ab77b3..5988f600 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=06-07 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v6]
On Wed, 14 Apr 2021 15:03:19 GMT, Alan Bateman wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added comment to System.out/err init.
>
> src/java.base/share/classes/java/lang/System.java line 166:
>
>> 164: * exists, {@link Charset#defaultCharset()} otherwise.
>> 165: */
>> 166: public static final PrintStream err = null;
>
> I still think we need something in System.in to link to Console::charset. If
> someone creates an InputStreamReader(System.in) then it will use the default
> charset, they should be using Console::reader for these cases.
Added some explanation to it.
-
PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v7]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added Console::charset() relation with System.in - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/9e323145..45ab77b3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=05-06 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v2]
On Tue, 13 Apr 2021 15:03:28 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this proposed fix for
>> https://bugs.openjdk.java.net/browse/JDK-8262108?
>>
>> As noted in a comment in that issue, the bug relates to the return value of
>> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The
>> implementation has started returning invalid values for the `AM_PM` field
>> after the "day period" support was added recently in the JDK as part of
>> https://bugs.openjdk.java.net/browse/JDK-8262108.
>>
>> The commit here adds a check in the internal implementation of the display
>> name handling logic, to special case the `AM_PM` field and properly convert
>> the display name array indexes (which is an internal detail) to valid values
>> that represent the `AM_PM` calendar field.
>>
>> The commit also has a new jtreg test case `CalendarDisplayNamesTest`
>> reproduces this issue and verifies the fix.
>>
>> After this fix was introduced, I ran the test in
>> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing
>> test case `NarrowNamesTest`. Looking at that test case, IMO, the current
>> testing in that `NarrowNamesTest` is incorrect and is probably what hid this
>> issue in the first place? To fix this, I have added an additional commit
>> which updates this test case to properly test the `AM_PM` field values.
>
> Jaikiran Pai has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - copyright year and @bug id reference in existing test
> - copyright year on source
Thanks for checking.
`CalendarNameProviderImpl` is an implementation detail, which is shared between
`j.u.Calendar` and `j.t.f.DateTimeFormatter(Builder)` class, where it provides
localized names for Calendar fields. For the field `Calendar.AM_PM`, it
includes not only am/pm translations, but also day periods translations as you
see. So blindly mapping day period translations to Calendar.AM/PM index is
incorrect. I'd rather modify the code like:
if (field == AM_PM && !javatime && i > PM) {
continue;
} else {
map.put(name, base + i);
}
-
PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v2]
On Tue, 13 Apr 2021 18:56:22 GMT, Lance Andersen wrote: >> Hi all, >> >> Please review the following patch which adds additional permissions needed >> for when JTREG upgrades to a newer version of TestNG. >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > TestNG updates Permission re-organization Looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3471
Re: RFR: 8264208: Console charset API [v6]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added comment to System.out/err init. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/cafde56a..9e323145 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 19:30:53 GMT, Joe Wang wrote: >> Although the code path is different, the logic to determine the encoding is >> not changed, as `sun.stdout/err.encoding` are only set if the VM is invoked >> from a terminal (in fact, there's a bug where they aren't set even in a >> terminal on unix env, which I fixed in this patch) which is the same >> condition in which `System.console()` returns the console. And for both >> cases, it will default to `Charset.defaultCharset()` if they are not >> available. >> >> Having said that, one regression test started to fail with this >> implementation change as follows: >> >> Exception in thread "main" java.util.ServiceConfigurationError: Locale >> provider adapter "CLDR"cannot be instantiated. >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:199) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.findAdapter(LocaleProviderAdapter.java:287) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.getAdapter(LocaleProviderAdapter.java:258) >> at java.base/java.text.NumberFormat.getInstance(NumberFormat.java:960) >> at >> java.base/java.text.NumberFormat.getNumberInstance(NumberFormat.java:518) >> at java.base/java.util.Scanner.useLocale(Scanner.java:1270) >> at java.base/java.util.Scanner.(Scanner.java:543) >> at java.base/java.util.Scanner.(Scanner.java:554) >> at TestConsole.main(TestConsole.java:54) >> Caused by: java.lang.reflect.InvocationTargetException >> at >> java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native >> Method) >> at >> java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) >> at >> java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) >> at >> java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) >> at >> java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:188) >> ... 8 more >> >> I haven't looked at it in detail but it seems that calling >> `System.console()` in `System.initPhase1()` is not allowed, as the module >> system is not there yet. So I reverted the implementation to the original >> and simply retained the description in `System.out/err` with a change to >> `determined by` to `equivalent to`. > > Thanks for the explanation and updates. It's a worthwhile exercise to attempt > to align things around the new method. A short note above line 2023 to record > this might be useful as well (sth. like it's eq to console != null ? > console.charset() : Charset.defaultCharset();). No need to create another > update if you decide to add the note. Thanks. Added some explanations as suggested. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 13:04:17 GMT, Alan Bateman wrote: > 1. I think method name "charset()" is too short. It's not called frequently. > This method name should explain functionality. As for this one, I am open for suggestions. I thought `consoel()` was concise, and analogous to `Charset.defaultCharset()`. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 02:34:15 GMT, Joe Wang wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reverted PrintStream changes > > src/java.base/share/classes/java/lang/System.java line 2020: > >> 2018: setIn0(new BufferedInputStream(fdIn)); >> 2019: setOut0(newPrintStream(fdOut, cs)); >> 2020: setErr0(newPrintStream(fdErr, cs)); > > It was getting from sun.stdout.encoding or sun.stderr.encoding. After the > change, when there is no console, it would be set with > Charset.defaultCharset(). Would that be an incompatible change? Although the code path is different, the logic to determine the encoding is not changed, as `sun.stdout/err.encoding` are only set if the VM is invoked from a terminal (in fact, there's a bug where they aren't set even in a terminal on unix env, which I fixed in this patch) which is the same condition in which `System.console()` returns the console. And for both cases, it will default to `Charset.defaultCharset()` if they are not available. Having said that, one regression test started to fail with this implementation change as follows: Exception in thread "main" java.util.ServiceConfigurationError: Locale provider adapter "CLDR"cannot be instantiated. at java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:199) at java.base/sun.util.locale.provider.LocaleProviderAdapter.findAdapter(LocaleProviderAdapter.java:287) at java.base/sun.util.locale.provider.LocaleProviderAdapter.getAdapter(LocaleProviderAdapter.java:258) at java.base/java.text.NumberFormat.getInstance(NumberFormat.java:960) at java.base/java.text.NumberFormat.getNumberInstance(NumberFormat.java:518) at java.base/java.util.Scanner.useLocale(Scanner.java:1270) at java.base/java.util.Scanner.(Scanner.java:543) at java.base/java.util.Scanner.(Scanner.java:554) at TestConsole.main(TestConsole.java:54) Caused by: java.lang.reflect.InvocationTargetException at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) at java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:188) ... 8 more I haven't looked at it in detail but it seems that calling `System.console()` in `System.initPhase1()` is not allowed, as the module system is not there yet. So I reverted the implementation to the original and simply retained the description in `System.out/err` with a change to `determined by` to `equivalent to`. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v5]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflected further review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/68db1a79..cafde56a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=03-04 Stats: 14 lines in 1 file changed: 6 ins; 2 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v2]
On Tue, 13 Apr 2021 15:03:28 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed fix for >> https://bugs.openjdk.java.net/browse/JDK-8262108? >> >> As noted in a comment in that issue, the bug relates to the return value of >> `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The >> implementation has started returning invalid values for the `AM_PM` field >> after the "day period" support was added recently in the JDK as part of >> https://bugs.openjdk.java.net/browse/JDK-8262108. >> >> The commit here adds a check in the internal implementation of the display >> name handling logic, to special case the `AM_PM` field and properly convert >> the display name array indexes (which is an internal detail) to valid values >> that represent the `AM_PM` calendar field. >> >> The commit also has a new jtreg test case `CalendarDisplayNamesTest` >> reproduces this issue and verifies the fix. >> >> After this fix was introduced, I ran the test in >> `test/jdk/java/util/Calendar/` and that showed up a failure in an existing >> test case `NarrowNamesTest`. Looking at that test case, IMO, the current >> testing in that `NarrowNamesTest` is incorrect and is probably what hid this >> issue in the first place? To fix this, I have added an additional commit >> which updates this test case to properly test the `AM_PM` field values. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - copyright year and @bug id reference in existing test > - copyright year on source Have you run regression tests in java.time? If I am not mistaken, your changes simply seem to nullify the day period support. - PR: https://git.openjdk.java.net/jdk/pull/3463
Re: RFR: 8264208: Console charset API [v2]
On Mon, 12 Apr 2021 21:12:08 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/io/Console.java line 397:
>>
>>> 395: /**
>>> 396: * Returns the {@link java.nio.charset.Charset Charset} object
>>> used in
>>> 397: * this {@code Console}.
>>
>> The Console is a singleton and the existing methods use "the console" so I
>> think we should do the same here.
>>
>> We'll need to add to the description of the System.{in,out,err} fields, I
>> don't mind if we do it as part of this PR or another issue.
>
> Javadoc updated. Instead of modifying System.out/err (System.in is not
> affected, as it does not convert b2c), I modified PrintStream javadoc.
Reverted the changes to PrintStream, as it defaults to
Charset.defaultCharset(). Added descriptions to System.out/err instead (and
modified the impl).
-
PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reverted PrintStream changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/c172d0a1..68db1a79 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=02-03 Stats: 50 lines in 2 files changed: 8 ins; 9 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v2]
On Sun, 11 Apr 2021 13:44:05 GMT, Alan Bateman wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflected the review comments.
>
> src/java.base/share/classes/java/io/Console.java line 397:
>
>> 395: /**
>> 396: * Returns the {@link java.nio.charset.Charset Charset} object used
>> in
>> 397: * this {@code Console}.
>
> The Console is a singleton and the existing methods use "the console" so I
> think we should do the same here.
>
> We'll need to add to the description of the System.{in,out,err} fields, I
> don't mind if we do it as part of this PR or another issue.
Javadoc updated. Instead of modifying System.out/err (System.in is not
affected, as it does not convert b2c), I modified PrintStream javadoc.
-
PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v3]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflecting the review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/8fd8f6e6..c172d0a1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=01-02 Stats: 33 lines in 2 files changed: 3 ins; 0 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v2]
Hi Bernd, On 4/9/21 5:21 PM, Bernd Eckenfels wrote: Hello, I like the API, it is useful, however not enough to replace the defaultCharset once the Change to UTF8 is done. You still need a way to query the platforms file encoding (especially on Windows). Initially I thought it would be beneficial to provide the method that returns so-called `platform` charset, but I am not so sure introducing it. The reason is that once JEP 400 is enabled, that method only serves to migrate the old apps in the new environment. And that's where the `COMPAT` system property would be utilized. If those apps have luxury to make source code changes, I would recommend migrating the code by giving the charset argument to the failing FileReader or alike. Also I wonder if the Javadoc needs to discuss platform aspects of console, especially System.out and LANG on unix vs. windows. I will add some descriptions to System.out/err in relation to Console, but how they map to platform's settings (LANG on Unix/System locale on Windows) is an implementation detail, and I don't think it should be be described in the spec. Naoto Gruss Bernd -- http://bernd.eckenfels.net Von: security-dev im Auftrag von Naoto Sato Gesendet: Friday, April 9, 2021 11:06:00 PM An: core-libs-dev@openjdk.java.net ; security-...@openjdk.java.net Betreff: Re: RFR: 8264208: Console charset API [v2] Please review the changes for the subject issue. This has been suggested in a recent discussion thread for the JEP 400 [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. A CSR has also been drafted, and comments are welcome [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflected the review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/d6db04bb..8fd8f6e6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=00-01 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v2]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflected the review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/d6db04bb..8fd8f6e6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3419=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3419=00-01 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v2]
On Fri, 9 Apr 2021 19:25:02 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflected the review comments.
>
> src/java.base/share/classes/java/io/Console.java line 404:
>
>> 402: *
>> 403: * @return A {@code Charset} object used in this {@code Console}.
>> 404: * @since 17
>
> A couple of minor comments:
> May replace {@code Charset} with @link;
> Add "the" to the sentence: The returned charset corresponds to "the" input...
> @return: javadoc guide suggests "do not capitalize and do not end with a
> period" when writing a phrase. But I guess for consistency in this class,
> it's okay to capitalize.
Thanks, Joe. Modified as suggested.
-
PR: https://git.openjdk.java.net/jdk/pull/3419
Integrated: 8264765: BreakIterator sees bogus sentence boundary in parenthesized “i.e.” phrase
On Thu, 8 Apr 2021 18:19:20 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. It is not actually related to > "parenthesized", but period-comma sequence was regarded as a break on a > backward traverse. This pull request has now been integrated. Changeset: 9ebc497b Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/9ebc497b Stats: 19 lines in 2 files changed: 14 ins; 0 del; 5 mod 8264765: BreakIterator sees bogus sentence boundary in parenthesized “i.e.” phrase Reviewed-by: joehw - PR: https://git.openjdk.java.net/jdk/pull/3400
RFR: 8264208: Console charset API
Please review the changes for the subject issue. This has been suggested in a recent discussion thread for the JEP 400 [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. A CSR has also been drafted, and comments are welcome [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. - Commit messages: - 8264208: Console charset API Changes: https://git.openjdk.java.net/jdk/pull/3419/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3419=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264208 Stats: 202 lines in 9 files changed: 174 ins; 17 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
RFR: 8264765: BreakIterator sees bogus sentence boundary in parenthesized “i.e.” phrase
Please review the fix to the subject issue. It is not actually related to "parenthesized", but period-comma sequence was regarded as a break on a backward traverse. - Commit messages: - 8264765: BreakIterator sees bogus sentence boundary in parenthesized “i.e.” phrase Changes: https://git.openjdk.java.net/jdk/pull/3400/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3400=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264765 Stats: 19 lines in 2 files changed: 14 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/3400.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3400/head:pull/3400 PR: https://git.openjdk.java.net/jdk/pull/3400
Re: RFR: 8261301: StringWriter.flush() is NOOP but documentation does not indicate it
On Wed, 7 Apr 2021 21:01:48 GMT, Brian Burkhalter wrote:
> The specification of the method `flush()` in the `java.io` classes
> `CharArrayWriter` and `StringWriter` is not explicit about the fact that the
> method has no effect. This request proposes to add to the specification of
> each flush() method the sentence
> The {@code flush} method of {@code } does nothing.
> The corresponding CSR is JDK-8264867.
Marked as reviewed by naoto (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/3383
Re: RFR: 8264512: jdk/test/jdk/java/util/prefs/ExportNode.java relies on default platform encoding
On Fri, 2 Apr 2021 21:45:58 GMT, Brian Burkhalter wrote: > This test emits to a `java.io.ByteArrayOutputStream` the contents of a > `java.utils.prefs.Preferences` node. The `UTF-8` character encoding is used > [1]. The `ByteArrayOutputStream` is then converted to a `String` using > `toString()` which uses the platform's default character set [2]. The > resulting `String` is then checked for the node names that it should and > should not contain. > > This change proposes to use `ByteArrayOutputStream.toString(String)` to > obtain the string [3] to maintain consistency of the encoding. It also adds > throwing a `RuntimeException` if the node string is not as expected. It is > unclear why this test was not already throwing such an exception. > > [1] > https://docs.oracle.com/en/java/javase/16/docs/api/java.prefs/java/util/prefs/Preferences.html#exportNode(java.io.OutputStream) > [2] > https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/io/ByteArrayOutputStream.html#toString() > [3] > https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/io/ByteArrayOutputStream.html#toString(java.lang.String) Looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3332
Integrated: 8264544: Case-insensitive comparison issue with supplementary characters.
On Thu, 1 Apr 2021 03:24:04 GMT, Naoto Sato wrote: > Please review the fix to the subject issue. Thanks to the contribution by > Chris Johnson. This pull request has now been integrated. Changeset: 6c145c47 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/6c145c47 Stats: 11 lines in 3 files changed: 3 ins; 0 del; 8 mod 8264544: Case-insensitive comparison issue with supplementary characters. Co-authored-by: Chris Johnson Reviewed-by: joehw, iris, alanb - PR: https://git.openjdk.java.net/jdk/pull/3300
Re: RFR: 8205502: Make exception message from AnnotationInvocationHandler more informative
On Thu, 1 Apr 2021 22:15:01 GMT, Joe Darcy wrote: > Simple change to make the exception/error messages more informative for > various malformed annotation situations. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3317
RFR: 8264544: Case-insensitive comparison issue with supplementary characters.
Please review the fix to the subject issue. Thanks to the contribution by Chris Johnson. - Commit messages: - 8264544: Case-insensitive comparison issue with supplementary characters. Changes: https://git.openjdk.java.net/jdk/pull/3300/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3300=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264544 Stats: 11 lines in 3 files changed: 3 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/3300.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3300/head:pull/3300 PR: https://git.openjdk.java.net/jdk/pull/3300
Re: Proposed: StringUTF16 bug fix with optimization - Part 1 of 2, StringUTF16 Patch
Hi Chris,
Thank you for your contribution. I believe this can be divided into two
parts, one is the bug in the current implementation, and the other is
the enhancement to refactor the whole implementation for performance. I
have created two JIRA issues for each:
https://bugs.openjdk.java.net/browse/JDK-8264544
https://bugs.openjdk.java.net/browse/JDK-8264545
The former one is really an embarassing one that I would want to fix
right away. I think the latter one need more eyes. Comments from others
are welcome.
Naoto
On 3/30/21 4:44 PM, Chris Johnson wrote:
Historically, the methods currently known as "compareToCI" and
"regionMatchesCI", and located in "java.lang.StringUTF16", have lacked
support for Supplementary Multilingual Plane code-points. (I've seen no
associated bug.)
On July 23, 2020 the first fix for the bug was committed. However, it
includes two simple bugs of its own. They're not much more than typos,
but they break some things nonetheless, as demonstrated by the unit
tests comprising part 2 of this contribution.
(Those two bugs: In "StringUTF16.compareToCIImpl", change statements
"cp1 -= cp1;" and "cp2 -= cp2;" to, respectively, "cp1 = -cp1;" and
"cp2 = -cp2;", and those bugs are history.)
The fixed "compareToCI" and "regionMatchesCI" methods in this patch are
different, however.[1] Notably:
1. Case-insensitive UTF-16 string comparison and equality-testing is
2.2 to 2.9 times faster than the current methods, depending on data
set composition. (So, meaningfully faster, but the degree varies
with the strings compared.)
2. 100% TestNG unit test coverage.
3. A utility method, "compareCodePointsIgnoringCase", created for these
methods increased their performance by 24%, and could, in the future,
be applied easily to "StringLatin1" methods "compareToCI*", and
"regionMatchesCI*". (My guess is the performance gain there will be
smaller, but still useful.)
This patch to "StringUTF16" may appear quite large. For better or worse
(your call, gentle reader), the vast majority of it is comments. The code
itself is minimal by comparison.
Thanks in advance for your consideration,
Chris
Chris W. Johnson
chriswjohnson@gmail.com
http://www.panojohnson.com/
Footnote:
1. Work on this code began around 4 years ago, when I failed to find a
bug report link on the OpenJDK page, and supposed the message to devs
might be something like "contribute or go away". Since then, life,
death, work, learning to build & test OpenJDK, registering as a
contributor, social anxiety, unit test development, benchmarking,
experimentation, and being unable to look at my own code without
seeing *something* to improve (especially considering it might end-up
in the JDK), are among the reasons for the long delay in attempting
this contribution.
Index: src/java.base/share/classes/java/lang/StringUTF16.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>US-ASCII
===
diff --git a/src/java.base/share/classes/java/lang/StringUTF16.java
b/src/java.base/share/classes/java/lang/StringUTF16.java
--- a/src/java.base/share/classes/java/lang/StringUTF16.java(revision
60819:ee1d592a9f5389725a0338a4b5dfcf4fc3fcf20c)
+++ b/src/java.base/share/classes/java/lang/StringUTF16.java(revision
60819+:ee1d592a9f53+)
@@ -41,7 +41,32 @@
import static java.lang.String.LATIN1;
final class StringUTF16 {
-
+
+/**
+ * Number of low-order bits storing the payload of Unicode surrogate
code-units.
+ * (The number of surrogate header bits is {@link Character#SIZE}
minus this value.)
+ */
+private static final intSURROGATE_PAYLOAD_BIT_COUNT = 10;
+
+/**
+ * The high six bits common to all high-surrogate code-units (bits
15-10) right-shifted into bits
+ * 5-0 of an "{@code int}" primitive. ({@code 0b1101_10**__
10 == 0b11_0110 == 0x36})
+ */
+private static final intSURROGATE_HI_HEADER_RIGHT_SHIFTED_10 =
Character.MIN_HIGH_SURROGATE >>> SURROGATE_PAYLOAD_BIT_COUNT; // == 0x36
+
+/**
+ * Produces a Supplementary Multilingual Plane code-point when added
to a valid surrogate-pair
+ * combined as follows: {@code (highSurrogate <<
SURROGATE_PAYLOAD_BIT_COUNT) + lowSurrogate}.
+ * The result is undefined if either would-be surrogate is invalid
(not a surrogate code-unit,
+ * or the wrong surrogate type [low instead of high, or vice versa]).
+ *
+ * @see Character#toCodePoint(char, char)
+ */
+private static final intSURROGATE_COMBO_TO_CODE_POINT_DELTA =
Character.MIN_SUPPLEMENTARY_CODE_POINT
+-
(Character.MIN_HIGH_SURROGATE << SURROGATE_PAYLOAD_BIT_COUNT)
+-
Character.MIN_LOW_SURROGATE; // -56_613_888
+
+
Integrated: 8262110: DST starts from incorrect time in 2038
On Tue, 23 Mar 2021 23:38:28 GMT, Naoto Sato wrote: > Please review the fix to the DST transition bug after the year 2037. The > logic had the side effect that it adjusted the dst offset every time the > method `getOffsets()` is issued. Only adjust the offset when issued with wall > time. This pull request has now been integrated. Changeset: 7284f013 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/7284f013 Stats: 75 lines in 2 files changed: 73 ins; 0 del; 2 mod 8262110: DST starts from incorrect time in 2038 8073446: TimeZone getOffset API does not return a dst offset between years 2038-2137 Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/3165
Re: RFR: 8263754: HexFormat 'fromHex' methods should be static
On Thu, 25 Mar 2021 20:08:14 GMT, Roger Riggs wrote: > A number of HexFormat methods converting from strings to numbers do not use > delimiter, prefix, suffix, and uppercase parameters and would be more > convenient if the methods were static. > > These APIs were added early in JDK 17 and are being updated before GA. > This PR updates existing uses in the JDK but there may be compiler warnings > in non-JDK source files. > >public boolean isHexDigit(int); >public int fromHexDigit(int); >public int fromHexDigits(java.lang.CharSequence); >public int fromHexDigits(java.lang.CharSequence, int, int); >public long fromHexDigitsToLong(java.lang.CharSequence); >public long fromHexDigitsToLong(java.lang.CharSequence, int, int); Looks good (with the suggestions by Claes). - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3205
RFR: 8262110: DST starts from incorrect time in 2038
Please review the fix to the DST transition bug after the year 2037. The logic had the side effect that it adjusted the dst offset every time the method `getOffsets()` is issued. Only adjust the offset when issued with wall time. - Commit messages: - Set time zone to the formatter. - Added eof - Added a test case for 8073446 - 8262110: DST starts from incorrect time in 2038 Changes: https://git.openjdk.java.net/jdk/pull/3165/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3165=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8262110 Stats: 75 lines in 2 files changed: 73 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3165.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3165/head:pull/3165 PR: https://git.openjdk.java.net/jdk/pull/3165
Re: RFR: 8263668: Update java.time to use instanceof pattern variable
On Wed, 24 Mar 2021 09:56:16 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.time` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick LGTM. Thanks for the cleanup! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: JDK-8259926: Error in jpackage sample usage in the help text
On Mon, 22 Mar 2021 20:40:09 GMT, Andy Herrick wrote: > JDK-8259926: Error in jpackage sample usage in the help text Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3132
Integrated: 8263890: Broken links to Unicode.org
On Fri, 19 Mar 2021 17:57:31 GMT, Naoto Sato wrote: > Fixed several broken links to Unicode.org. This pull request has now been integrated. Changeset: 96e5c3f1 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/96e5c3f1 Stats: 37 lines in 8 files changed: 4 ins; 0 del; 33 mod 8263890: Broken links to Unicode.org Reviewed-by: redestad, joehw, iris - PR: https://git.openjdk.java.net/jdk/pull/3093
Re: RFR: 8263890: Broken links to Unicode.org [v2]
On Fri, 19 Mar 2021 18:43:30 GMT, Joe Wang wrote: > Some minor comments. Thanks, Joe. Addressed them as suggested. - PR: https://git.openjdk.java.net/jdk/pull/3093
Re: RFR: 8263890: Broken links to Unicode.org [v2]
> Fixed several broken links to Unicode.org. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressed review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3093/files - new: https://git.openjdk.java.net/jdk/pull/3093/files/2b32b86a..37093a4d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3093=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3093=00-01 Stats: 24 lines in 6 files changed: 4 ins; 0 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/3093.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3093/head:pull/3093 PR: https://git.openjdk.java.net/jdk/pull/3093
Re: RFR: 8263658: Use the blessed modifier order in java.base
On Fri, 19 Mar 2021 18:23:00 GMT, Alex Blewitt wrote: > Additional changes found in `java.base` of `final private` -> `private > final`. Filed with existing bug because it's the same module; can change to a > different bug number if required. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3094
RFR: 8263890: Broken links to Unicode.org
Fixed several broken links to Unicode.org. - Commit messages: - 8263890: Broken links to Unicode.org Changes: https://git.openjdk.java.net/jdk/pull/3093/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3093=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263890 Stats: 22 lines in 8 files changed: 0 ins; 0 del; 22 mod Patch: https://git.openjdk.java.net/jdk/pull/3093.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3093/head:pull/3093 PR: https://git.openjdk.java.net/jdk/pull/3093
Re: RFR: 8261673: Move javadoc for the lookup mechanism to module-info [v2]
On Tue, 16 Mar 2021 21:39:26 GMT, Joe Wang wrote: >> Consolidate and move javadoc for the lookup mechanism to the module summary. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typos: s/XMLEventFactory/XMLInputFactory > s/XMLEventFactory/XMLOutputFactory Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3020
Re: RFR: 8261673: Move javadoc for the lookup mechanism to module-info
On Tue, 16 Mar 2021 00:52:24 GMT, Joe Wang wrote:
> Consolidate and move javadoc for the lookup mechanism to the module summary.
src/java.xml/share/classes/javax/xml/stream/XMLInputFactory.java line 172:
> 170:* Creates a new instance of the factory. This method uses the
> 171:* JAXP Lookup
> Mechanism
> 172:* to determine the {@code XMLEventFactory} implementation class to
> load.
Should it be `XMLInputFactory`?
src/java.xml/share/classes/javax/xml/stream/XMLOutputFactory.java line 149:
> 147:* Creates a new instance of the factory. This method uses the
> 148:* JAXP Lookup
> Mechanism
> 149:* to determine the {@code XMLEventFactory} implementation class to
> load.
And this one as `XMLOutputFactory`?
-
PR: https://git.openjdk.java.net/jdk/pull/3020
Re: RFR: 8263556: remove `@modules java.base` from tests
On Sat, 13 Mar 2021 20:26:42 GMT, Igor Ignatyev wrote: > Hi all, > > could you please review this trivial cleanup? > from JBS: > >> jtreg `@modules X` directive does two things: >> - exclude a test from execution if JDK under test doesn't have module X >> - if JDK under test has module X, make sure it's resolved >> >> both these things have no sense for `java.base` module as it's always >> available and is always resolved. > > > Thanks, > -- Igor Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2990
Re: RFR: 8262351: Extra '0' in java.util.Formatter for '%012a' conversion with a sign character [v2]
On Wed, 10 Mar 2021 02:31:28 GMT, Ian Graves wrote:
>> This fixes a zero-adding issue observed in the hex float conversion.
>
> Ian Graves has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updating Formatter copyright date
Looks good to me.
BTW apart from this issue, I just noticed that in the chart in the `Formatter`
class spec, there are two spaces (two NBSPs) for the `extra space` flag:
* ''
* 'u0020'
* Requires the output to include a single extra space
* ('u0020') for non-negative values.
*
* If both the {@code '+'} and '' flags are
given
* then an {@link IllegalFormatFlagsException} will be thrown.
```
This might be fixed sometime.
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2881
Re: RFR: 8263038: Optimize String.format for simple specifiers [v4]
On Mon, 8 Mar 2021 20:42:21 GMT, Claes Redestad wrote:
>> This patch optimizes String.format expressions that uses trivial specifiers.
>> In the JDK, the most common variation of String.format is a variation of
>> format("foo: %s", s), which gets a significant speed-up from this.
>>
>> Various other cleanups and minor improvements reduce overhead further and
>> ensure we get a small gain also for more complex format strings.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Better words
Looks good!
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2830
Re: RFR: 8263190: Update java.io, java.math, and java.text to use instanceof pattern variable
On Mon, 8 Mar 2021 18:48:30 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.io`, > `java.math`, and `java.text` packages to make use of the `instanceof` pattern > variable? > > Kind regards, > Patrick Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2879
Re: RFR: 8263038: Optimize String.format for simple specifiers [v3]
On Mon, 8 Mar 2021 18:52:19 GMT, Claes Redestad wrote:
>> This patch optimizes String.format expressions that uses trivial specifiers.
>> In the JDK, the most common variation of String.format is a variation of
>> format("foo: %s", s), which gets a significant speed-up from this.
>>
>> Various other cleanups and minor improvements reduce overhead further and
>> ensure we get a small gain also for more complex format strings.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use a BOM as a sentinel for uninitialized zero
src/java.base/share/classes/java/util/Formatter.java line 1936:
> 1934: private IOException lastException;
> 1935:
> 1936: // Non-unicode code point value used to mark zero as uninitialized
Sorry for being anal here, but BOM is a "noncharacter", a valid Unicode code
point. So "noncharacter" over "Non-unicode code point" seems appropriate.
http://www.unicode.org/faq/private_use.html#nonchar1
-
PR: https://git.openjdk.java.net/jdk/pull/2830
Re: RFR: 8263038: Optimize String.format for simple specifiers [v2]
On Mon, 8 Mar 2021 16:00:27 GMT, Claes Redestad wrote:
>> This patch optimizes String.format expressions that uses trivial specifiers.
>> In the JDK, the most common variation of String.format is a variation of
>> format("foo: %s", s), which gets a significant speed-up from this.
>>
>> Various other cleanups and minor improvements reduce overhead further and
>> ensure we get a small gain also for more complex format strings.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Lazily evaluate zero
src/java.base/share/classes/java/util/Formatter.java line 2447:
> 2445: private char zero() {
> 2446: char zero = this.zero;
> 2447: if (zero == 0) {
U+ is a valid character. Although it's almost no possibility where any
locale assigns it as the zero in it, theoretically it is possible. It can be
avoided by comparing it with a non-character, such as BOM (U+FFFE)
-
PR: https://git.openjdk.java.net/jdk/pull/2830
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Fri, 5 Mar 2021 18:53:29 GMT, Claes Redestad wrote: >> This patch refactors Locale.getDefault(Category) so that the volatile field >> holding the Locale is typically only read once. This has a small performance >> advantage, and might be more robust if initialization is racy. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix omitted synchronized Looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263091: Remove CharacterData.isOtherUppercase/-Lowercase
On Fri, 5 Mar 2021 14:24:34 GMT, Claes Redestad wrote: > This patch removes the CharacterData.isOtherUppercase and isOtherLowercase > methods. It also exploits the fact that isOtherUppercase is always false for > all codepoints in the CharacterDataLatin1 range for a small speed-up. > > I have no means to test if this is correct on PPC, which has intrinsics for > isLowerCase/isUpperCase, but unless I'm reading the code wrong the intrinsic > for isLowerCase on PPC already appears to effectively do the fused logic of > isLowerCase(ch) || isOtherLowerCase(ch) since it handles the two values where > isLowerCase and isOtherLowercase disagrees (0xaa, 0xba), which means this > change should make the intrinsic and the java code be in better agreement. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2846
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category)
On Fri, 5 Mar 2021 14:14:14 GMT, Claes Redestad wrote:
> This patch refactors Locale.getDefault(Category) so that the volatile field
> holding the Locale is typically only read once. This has a small performance
> advantage, and might be more robust if initialization is racy.
src/java.base/share/classes/java/util/Locale.java line 959:
> 957: }
> 958:
> 959: private static Locale getDisplayLocale() {
Should this be `synchronized`?
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8261670: Add javadoc for the XML processing limits [v4]
On Mon, 1 Mar 2021 23:59:11 GMT, Joe Wang wrote: >> Add the documentation for XML processing limits to module summary. The >> limits were previously documented in Java tutorial and guide. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Change the description of the property table to indicate that the list of > the properties is not exhaustive Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2732
Re: RFR: 8261670: Add javadoc for the XML processing limits [v3]
On Fri, 26 Feb 2021 08:04:02 GMT, Joe Wang wrote: >> Add the documentation for XML processing limits to module summary. The >> limits were previously documented in Java tutorial and guide. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > s/NumericFormatException/NumberFormatException, remove default id Looks good. Thanks for making the changes. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2732
Re: RFR: JDK-8262428: doclint warnings in java.xml module
On Thu, 25 Feb 2021 22:41:46 GMT, Jonathan Gibbons wrote: > Please review a small doc fix to remove some superfluous `` tags and an > erroneous `` tag, all reported by doclint.. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2733
Re: RFR: 8261670: Add javadoc for the XML processing limits
On Thu, 25 Feb 2021 22:04:41 GMT, Joe Wang wrote: > Add the documentation for XML processing limits to module summary. The limits > were previously documented in Java tutorial and guide. src/java.xml/share/classes/module-info.java line 231: > 229: * > 230: * A positive integer. A value less than or equal to 0 indicates no > limit. > 231: * If the value is not an integer, a NumericFormatException is thrown. Is it a ```NumberFormatException```? Also, @link might be useful here. src/java.xml/share/classes/module-info.java line 233: > 231: * If the value is not an integer, a NumericFormatException is thrown. > 232: * > 233: * 64000 There are other ```Default``` ids in other rows. Should they be distinct? Or rather needed? - PR: https://git.openjdk.java.net/jdk/pull/2732
Withdrawn: 8262066: ProblemList java/util/Locale/LocaleProvidersRun.java
On Sat, 20 Feb 2021 00:44:29 GMT, Naoto Sato wrote: > The subject test case is failing under the JMS-enabled environment. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/2657
Re: RFR: 8262066: ProblemList java/util/Locale/LocaleProvidersRun.java [v2]
On Sat, 20 Feb 2021 02:16:49 GMT, Daniel D. Daugherty wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Limit the exclustion to linux-x64 > > Changes requested by dcubed (Reviewer). As the original issue was resolved, no need to problem list this. - PR: https://git.openjdk.java.net/jdk/pull/2657
Integrated: 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null"
On Tue, 23 Feb 2021 02:09:01 GMT, Naoto Sato wrote: > Please review the fix to this test case failure that occurs with the usage > tracker enabled JRE. This pull request has now been integrated. Changeset: 9d9ad969 Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/9d9ad969 Stats: 31 lines in 2 files changed: 16 ins; 7 del; 8 mod 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null" Reviewed-by: joehw, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/2683
Re: RFR: 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null" [v2]
> Please review the fix to this test case failure that occurs with the usage > tracker enabled JRE. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflecting review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2683/files - new: https://git.openjdk.java.net/jdk/pull/2683/files/bfa3a8e5..4d82e8aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2683=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2683=00-01 Stats: 9 lines in 1 file changed: 1 ins; 2 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2683/head:pull/2683 PR: https://git.openjdk.java.net/jdk/pull/2683
Re: RFR: 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null"
On Tue, 23 Feb 2021 09:49:47 GMT, Daniel Fuchs wrote:
>> Please review the fix to this test case failure that occurs with the usage
>> tracker enabled JRE.
>
> test/jdk/java/util/Locale/LocaleProviders.java line 416:
>
>> 414: // Set the root logger on loading the logging class
>> 415: public static class LogConfig {
>> 416: final static LogRecord[] lra = new LogRecord[1];
>
> I would suggest to use some multi-thread safe container rather than a simple
> array to store the LogRecord. For instance - a CopyOnWriteArrayList - or
> something like that. Also you may want to harden the test by allowing for the
> possibility that some other logging might have occurred, and search the list
> for the record you expect rather than assuming it will be the first and only
> one.
> Otherwise looks good!
Thanks, Daniel. Will update the fix as you suggested.
-
PR: https://git.openjdk.java.net/jdk/pull/2683
RFR: 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null"
Please review the fix to this test case failure that occurs with the usage tracker enabled JRE. - Commit messages: - 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null" Changes: https://git.openjdk.java.net/jdk/pull/2683/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2683=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261919 Stats: 31 lines in 2 files changed: 17 ins; 7 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/2683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2683/head:pull/2683 PR: https://git.openjdk.java.net/jdk/pull/2683
Re: RFR: 8262066: ProblemList java/util/Locale/LocaleProvidersRun.java [v2]
> The subject test case is failing under the JMS-enabled environment. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Limit the exclustion to linux-x64 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2657/files - new: https://git.openjdk.java.net/jdk/pull/2657/files/7c9cf40e..6a6bc241 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2657=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2657=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2657.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2657/head:pull/2657 PR: https://git.openjdk.java.net/jdk/pull/2657
Re: RFR: 8262066: ProblemList java/util/Locale/LocaleProvidersRun.java
On Sat, 20 Feb 2021 02:16:43 GMT, Daniel D. Daugherty wrote: >> The subject test case is failing under the JMS-enabled environment. > > test/jdk/ProblemList.txt line 799: > >> 797: # jdk_util >> 798: >> 799: java/util/Locale/LocaleProvidersRun.java8261919 >> generic-all > > This only needs to be ProblemListed on linux-x64. It's not platform specific, but specific to the environment that is usage tracker enabled. Thus I put it as 'generic'. - PR: https://git.openjdk.java.net/jdk/pull/2657
Re: RFR: 8262041: javax/xml/jaxp/unittest/common/prettyprint/PrettyPrintTest.java fails after JDK-8260858
On Fri, 19 Feb 2021 22:12:35 GMT, Joe Wang wrote: > A quick fix to the test, removing Windows carriage return in the result. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2656
Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v3]
On Fri, 12 Feb 2021 17:25:00 GMT, Brian Burkhalter wrote: >> Please review this clarification of the specification of the method >> `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the >> behavior of the method is made clear for the case when the `Reader` is >> already at the end of its stream when the method is invoked. A corresponding >> CSR will be filed. Also, the change includes an update to an existing test >> in order to verify that the specification change reflects actual behavior. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8247918: Remove verbiage describing impossible behavior in CharArrayReader > and StringReader Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2274
Integrated: 8261728: SimpleDateFormat should link to DateTimeFormatter
On Wed, 17 Feb 2021 19:34:47 GMT, Naoto Sato wrote: > Please review this simple doc fix. A CSR will be filed accordingly. This pull request has now been integrated. Changeset: 8a1c712c Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/8a1c712c Stats: 8 lines in 2 files changed: 6 ins; 0 del; 2 mod 8261728: SimpleDateFormat should link to DateTimeFormatter Reviewed-by: bpb, rriggs, lancea, iris - PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v4]
On Thu, 18 Feb 2021 23:16:00 GMT, Claes Redestad wrote: >> This patch exposes a couple of intrinsics used by String to speed up ASCII >> checking and byte[] -> char[] inflation, which can be used by latin1 and >> ASCII-compatible CharsetDecoders to speed up decoding operations. >> >> - Fast-path implemented for all standard charsets, with up to 10x >> performance improvements in microbenchmarks reading Strings from >> ByteArrayInputStream. >> - Cleanup of StreamDecoder/-Encoder with some minor improvements when >> interpreting >> - Reworked creation of JavaLangAccess to be safely published for >> CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and >> these encoders are created during System.initPhase1 the current sequence >> caused the initialization to became unstable and a few tests were >> consistently getting an NPE. >> >> Testing: tier1-3 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert JavaLangAccessImpl changes Marked as reviewed by naoto (Reviewer). src/java.base/share/classes/java/lang/System.java line 1988: > 1986: // might initialize CharsetDecoders that rely on it > 1987: setJavaLangAccess(); > 1988: Good to see the init issue has been resolved with a simple reordering. - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v2]
On Thu, 18 Feb 2021 14:49:20 GMT, Roger Riggs wrote: >>> >>> >>> The table is informative and should not be construed as specification. >>> The wording "has supported" should be sufficient. >> >> If this is not specification then doesn't that imply that any provider of >> any version of OpenJDK would be free to support, or not, whatever version of >> Unicode that they chose? Surely a minimum supported version must be part of >> the platform specification? > > The current version of Unicode is specified in a normative statement just > before the table. > "Character information is based on the Unicode Standard, version 13.0." > > The table is not a specification of past revisions. RIght. And each Java SE release's spec has the same sentence with the version replaced. In fact, vendors cannot incorporate arbitrary Unicode versions as it would involve API changes. If they wanted to do so, an MR should have to be released. - PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v3]
On Wed, 17 Feb 2021 23:10:57 GMT, Brian Burkhalter wrote: >> Please review this minor specification update to highlight that >> `File.renameTo(File)` does not modify the `File` instance on which the >> method is invoked. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 6245663: Update copyright year. Thanks. Looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2618
Re: RFR: 6245663: (spec) File.renameTo(File) changes the file-system object, not the File instance [v2]
On Wed, 17 Feb 2021 22:12:06 GMT, Brian Burkhalter wrote:
>> Please review this minor specification update to highlight that
>> `File.renameTo(File)` does not modify the `File` instance on which the
>> method is invoked.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 6245663: Mention 'dest' parameter in the added doc
src/java.base/share/classes/java/io/File.java line 1381:
> 1379: * that the rename operation was successful. As instances of
> {@code File}
> 1380: * are immutable, the abstract pathname represented by this {@code
> File}
> 1381: * object does not itself change although the filesystem object it
> denoted
I guess you meant `file` object here, instead of `filesystem`?
-
PR: https://git.openjdk.java.net/jdk/pull/2618
Integrated: 8261621: Delegate Unicode history from JLS to j.l.Character
On Fri, 12 Feb 2021 02:50:35 GMT, Naoto Sato wrote: > Please review this doc fix to j.l.Character, which now includes the table of > the history of supported Unicode versions. A corresponding CSR will be filed > accordingly. This pull request has now been integrated. Changeset: ea5bf45c Author: Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/ea5bf45c Stats: 38 lines in 1 file changed: 36 ins; 0 del; 2 mod 8261621: Delegate Unicode history from JLS to j.l.Character Reviewed-by: bpb, joehw, rriggs, darcy - PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter [v2]
On Wed, 17 Feb 2021 20:04:33 GMT, Lance Andersen wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Made the additional text an @apiNote > > Hi Naoto, > > Looks good. > > Any thoughts to making the new wording stick out more perhaps via a something > like "Note: Consider" and bolding "Note:" > > > Not sure if it is @apiNote worthy but I guess that is an option also? > Not sure if it is @APinote worthy but I guess that is an option also? Good point. Modified. - PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter [v2]
> Please review this simple doc fix. A CSR will be filed accordingly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Made the additional text an @apiNote - Changes: - all: https://git.openjdk.java.net/jdk/pull/2616/files - new: https://git.openjdk.java.net/jdk/pull/2616/files/d840b562..c93badb4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2616=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2616=00-01 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2616.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2616/head:pull/2616 PR: https://git.openjdk.java.net/jdk/pull/2616
RFR: 8261728: SimpleDateFormat should link to DateTimeFormatter
Please review this simple doc fix. A CSR will be filed accordingly. - Commit messages: - 8261728: SimpleDateFormat should link to DateTimeFormatter Changes: https://git.openjdk.java.net/jdk/pull/2616/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2616=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261728 Stats: 8 lines in 2 files changed: 4 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2616.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2616/head:pull/2616 PR: https://git.openjdk.java.net/jdk/pull/2616
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v4]
> Please review this doc fix to j.l.Character, which now includes the table of > the history of supported Unicode versions. A corresponding CSR will be filed > accordingly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added a text about the variations from the base Unicode versions. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2538/files - new: https://git.openjdk.java.net/jdk/pull/2538/files/b94b41c8..c90cd663 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2538=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2538=02-03 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2538.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2538/head:pull/2538 PR: https://git.openjdk.java.net/jdk/pull/2538
Re: RFR: 8261744: Implement CharsetDecoder ASCII and latin-1 fast-paths [v2]
On Mon, 15 Feb 2021 15:19:01 GMT, Claes Redestad wrote: >> This patch exposes a couple of intrinsics used by String to speed up ASCII >> checking and byte[] -> char[] inflation, which can be used by latin1 and >> ASCII-compatible CharsetDecoders to speed up decoding operations. >> >> - Fast-path implemented for all standard charsets, with up to 10x >> performance improvements in microbenchmarks reading Strings from >> ByteArrayInputStream. >> - Cleanup of StreamDecoder/-Encoder with some minor improvements when >> interpreting >> - Reworked creation of JavaLangAccess to be safely published for >> CharsetDecoders/-Encoders used for setting up System.out/in. As JLA and >> these encoders are created during System.initPhase1 the current sequence >> caused the initialization to became unstable and a few tests were >> consistently getting an NPE. >> >> Testing: tier1-3 > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert rem assertions Great improvement! Looks good to me overall. I wondered about the changes in JLA as Alan already mentioned. src/java.base/share/classes/java/lang/String.java line 1017: > 1015: * @return the number of bytes successfully decoded, at most len > 1016: */ > 1017: /* package-private */ Some more explanation would be helpful here, as it is accessed from System for shared secrets. src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 48: > 46: import jdk.internal.module.ServicesCatalog; > 47: import jdk.internal.reflect.ConstantPool; > 48: import jdk.internal.vm.annotation.IntrinsicCandidate; Not needed. - PR: https://git.openjdk.java.net/jdk/pull/2574
Re: RFR: 8261621: Delegate Unicode history from JLS to j.l.Character [v3]
> Please review this doc fix to j.l.Character, which now includes the table of > the history of supported Unicode versions. A corresponding CSR will be filed > accordingly. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Removed empty tag - 8261621: Delegate Unicode history from JLS to j.l.Character - Changes: https://git.openjdk.java.net/jdk/pull/2538/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2538=02 Stats: 36 lines in 1 file changed: 34 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2538.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2538/head:pull/2538 PR: https://git.openjdk.java.net/jdk/pull/2538
