Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v3]
On Mon, 10 Jun 2024 20:46:46 GMT, Naoto Sato wrote: >> Damon Nguyen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Revert updated translation > > Did not look at each for translation accuracy (did some for Japanese), but > looks good overall. > It's great to see diffs in native languages, rather than those cryptic > `\u` notation! > An interesting note after looking at Phil's comment on the > serviceui.properties change is that there is no Japanese update for that > file. The translations on > [here](https://github.com/openjdk/jdk/commit/c7d2a5c1c4e86955100f4c40170dc25222abd07f#diff-38c26a463e174224c6b9fc5007f3058482e533dc0f60f3275e21f9f2fc9acf8e) > from the output bins update look good to me but maybe @naotoj should double > check in case? I can confirm that English "Out trays:" to "出力トレイ():" is OK - PR Comment: https://git.openjdk.org/jdk/pull/19609#issuecomment-2161765683
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v2]
On Mon, 10 Jun 2024 19:45:14 GMT, Damon Nguyen wrote:
>> src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/msg/XMLMessages_de.properties
>> line 278:
>>
>>> 276: UndeclaredElementInContentSpec = Contentmodell des Elements
>>> "{0}" verweist auf das nicht deklarierte Element "{1}".
>>> 277: UniqueNotationName = Deklaration für die Notation "{0}" ist
>>> nicht eindeutig. Ein jeweiliger Name darf nicht in mehreren
>>> Notationsdeklarationen deklariert werden.
>>> 278: ENTITYFailedInitializeGrammar = ENTITYDatatype-Validator:
>>> Nicht erfolgreich. Initialisierungsmethode muss mit einer gültigen
>>> Grammatikreferenz aufgerufen werden. \t
>>
>> It looks like the _zh_CN_ file should also have the `\t` removed, but such
>> changes are done by the translation tool. I think in this case, we should
>> manually remove it, and then file a bug against the translation tool.
>
> Yeah, I believe you're right. Noted as something else to file against the
> translation tool.
Just out of curiosity, do we know why `\t` was attached in the first place? I
don't think it is with the original English properties.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1635231459
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update
On Fri, 7 Jun 2024 22:46:44 GMT, Damon Nguyen wrote: > This issue is responsible for updating the translations of all the > localize(able) resources in the JDK. Primarily, the changes between JDK 22 > RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated. > > The translation tool adjusted some definitions, which causes some changes in > localized files where the source file had no changes. This causes some words > being reverted from localized languages to English, and some had its > definitions changed. > > Alternatively, the diffs are viewable here and was generated using Jonathan > Gibbons' diff tool for l10n: > https://cr.openjdk.org/~dnguyen/output2/ Did not look at each for translation accuracy (did some for Japanese), but looks good overall. It's great to see diffs in native languages, rather than those cryptic `\u` notation! - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19609#pullrequestreview-2108722631
Re: RFR: 8333358: java/io/IO/IO.java test fails intermittently
On Mon, 10 Jun 2024 12:29:32 GMT, Pavel Rappo wrote: > Please review this fix for an intermittent test failure. > > On some configurations, the default `expect` timeout of 10 seconds is > insufficient. It is increased to 20; it's hard to imagine a configuration for > which that new value would still be insufficient, but we'll see. > > Aside from that, test-generated diagnostics are improved: the version of the > `expect` command and the duration of each test method are recorded. > `output.exp` is modified for robustness and clear indication of the timeout > condition. > > I was reminded, out-of-band, that test timeouts should be scaled with > `test.timeout.factor`. I propose to integrate this PR first and then > separately update all the affected tests to scale their `expect` timeouts. LGTM test/jdk/java/io/IO/IO.java line 190: > 188: > 189: > 190: // adapted from > https://raw.githubusercontent.com/junit-team/junit5/main/documentation/src/test/java/example/timing/TimingExtension.java Probably better to refer to the user guide on the web (https://junit.org/junit5/docs/snapshot/user-guide/#extensions-lifecycle-callbacks-before-after-execution) - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19627#pullrequestreview-2108625709 PR Review Comment: https://git.openjdk.org/jdk/pull/19627#discussion_r1633749196
Integrated: 8332161: Test restoring echo in the Console implementation (java.base)
On Mon, 20 May 2024 18:08:31 GMT, Naoto Sato wrote: > This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. This pull request has now been integrated. Changeset: 25ad8623 Author: Naoto Sato URL: https://git.openjdk.org/jdk/commit/25ad86234a7cd6e606b273f3e63351aa07c567a3 Stats: 154 lines in 2 files changed: 154 ins; 0 del; 0 mod 8332161: Test restoring echo in the Console implementation (java.base) Reviewed-by: joehw, prappo - PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v12]
On Thu, 6 Jun 2024 21:49:35 GMT, Naoto Sato wrote: >> This test intends to verify the behavior of JdkConsole for the java.base >> module, wrt restoring the echo. The test assumes the internal methods that >> sets/gets the echo status of the platform. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Loosening the timeout value Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/19315#issuecomment-2155153612
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v12]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Loosening the timeout value - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/c6476270..df5f8f91 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=11 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v11]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Restored classpath - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/0e72fe46..c6476270 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=10 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=09-10 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
On Thu, 6 Jun 2024 17:08:45 GMT, Naoto Sato wrote: >> test/jdk/java/io/Console/RestoreEchoTest.java line 72: >> >>> 70: "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED", >>> 71: "-Djdk.console=java.base", >>> 72: "-classpath", testClasses, >> >> Consider this. If we remove `-classpath` (and `var testClasses`), not only >> will nothing break, but we'll be also able to use JUnit assertions and >> assumptions in `main` instead of manual check-then-throw. This will work >> because the expect-process will inherit the environment, which captures >> `CLASSPATH` ( see >> https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/ProcessBuilder.html#start() >> ). >> >> Again, the above is just something to consider. For all I know, you might've >> considered it already and rejected. > > I haven't considered that. Removed. Turned out that removing the classpath ends up not finding the test class: Error: Could not find or load main class RestoreEchoTest Caused by: java.lang.ClassNotFoundException: RestoreEchoTest ]; - PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629984403
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
On Thu, 6 Jun 2024 09:05:23 GMT, Pavel Rappo wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removed unnecessary add-opens > > test/jdk/java/io/Console/RestoreEchoTest.java line 66: > >> 64: OutputAnalyzer output = ProcessTools.executeProcess( >> 65: "expect", >> 66: "-n", > > What does `-n` do? It is for not reading the user's expect settings (`~/.expect.rc` if any) > test/jdk/java/io/Console/RestoreEchoTest.java line 72: > >> 70: "--add-opens=java.base/jdk.internal.io=ALL-UNNAMED", >> 71: "-Djdk.console=java.base", >> 72: "-classpath", testClasses, > > Consider this. If we remove `-classpath` (and `var testClasses`), not only > will nothing break, but we'll be also able to use JUnit assertions and > assumptions in `main` instead of manual check-then-throw. This will work > because the expect-process will inherit the environment, which captures > `CLASSPATH` ( see > https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/ProcessBuilder.html#start() > ). > > Again, the above is just something to consider. For all I know, you might've > considered it already and rejected. I haven't considered that. Removed. > test/jdk/java/io/Console/restoreEcho.exp line 57: > >> 55: test "$rpprompt" "$rpinput" "-echo" "$rpexpected" >> 56: # See if the initialEcho is restored with `stty -a` >> 57: expect " $initialEcho " > > If you leave out those whitespace characters inside the quotes and > `$initialEcho` expands to `-echo`, it will be treated as an option to > `expect`, right? If so, consider this instead: > > expect -- $initialEcho > > But more importantly: could a test match `echo` in `-echo` and therefore > falsely pass? The spaces before/after `$initialEcho` are exactly to distinguish "echo" from "-echo", otherwise the test falsely succeeds as you pointed out. Although the test works as expected as it is, adding `--` would be safer. - PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915235 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915623 PR Review Comment: https://git.openjdk.org/jdk/pull/19315#discussion_r1629915890
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v10]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/c583c633..0e72fe46 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=09 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=08-09 Stats: 11 lines in 2 files changed: 1 ins; 6 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v9]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Removed unnecessary add-opens - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/1c0ab8eb..c583c633 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=08 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=07-08 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization [v3]
On Wed, 5 Jun 2024 12:36:33 GMT, Jan Lahoda wrote:
>> Consider these two programs:
>>
>>
>> public class SystemPrint {
>> public static void main(String... args) {
>> System.err.println("Hello!");
>> }
>> }
>>
>> and:
>>
>> public class IOPrint {
>> public static void main(String... args) {
>> java.io.IO.println("Hello!");
>> }
>> }
>>
>>
>> They do the same conceptual thing - write a text to the output. But,
>> `IO.println` delegates to `Console.println`, which then delegates to a
>> `Console` backend, and the default backend is currently based on JLine.
>>
>> The issues is that JLine takes a quite a long time to initialize, and in a
>> program like this, JLine is not really needed - it is used to provide better
>> editing experience when reading input, but there's no reading in these
>> programs.
>>
>> For example, on my computer:
>>
>> $ time java -classpath /tmp SystemPrint
>> Hello!
>>
>> real0m0,035s
>> user0m0,019s
>> sys 0m0,019s
>>
>> $ time java -classpath /tmp --enable-preview IOPrint
>> Hello!
>>
>> real0m0,165s
>> user0m0,324s
>> sys 0m0,042s
>>
>>
>> The proposal herein is to delegate to the simpler `Console` backend from
>> `java.base` as long as the user only uses methods that print to output, and
>> switch to the JLine delegate when other methods (typically input) is used.
>> Note that while technically `writer()` is a method doing output, it will
>> force JLine initialization to avoid possible problems if the client caches
>> the writer and uses it after switching the delegates.
>>
>> With this patch, I can get timing like this:
>>
>> $ time java --enable-preview -classpath /tmp/ IOPrint
>> Hello!
>>
>> real0m0,051s
>> user0m0,038s
>> sys 0m0,020s
>>
>>
>> which seems much more acceptable.
>>
>> There is also #19467, which may reduce the time further.
>>
>> A future work might focus on making JLine initialize faster, but avoiding
>> JLine initialization in case where we don't need it seems like a good step
>> to me in any case.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Correctly reflecting review feedback
LGTM. Thanks for the changes.
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19479#pullrequestreview-2099801902
Re: RFR: 8333456: CompactNumberFormat integer parsing fails when string has no suffix [v2]
On Tue, 4 Jun 2024 20:43:09 GMT, Justin Lu wrote:
>> Please review this PR which handles incorrect CompactNumberFormat integer
>> only parsing when there is no suffix.
>>
>> See the following snippet,
>>
>>
>> var fmt = NumberFormat.getCompactNumberInstance(Locale.US,
>> NumberFormat.Style.SHORT)
>> fmt.setParseIntegerOnly(true)
>> fmt.parse("5K") // returns 5000
>> fmt.parse("50.00") // returns 50
>> fmt.parse("5") // unexpectedly throws StringIndexOutOfBoundsException
>> fmt.parse("5 ") // returns 5
>>
>>
>> Within the `parse` method, there is code that advances `position` past the
>> fractional portion to find the suffix when `parseIntegerOnly()` is true.
>> However, if a valid string input is given with no suffix,
>> `DecimalFormat.subparseNumber()` will fully iterate through the string and
>> `position` will be equal to the length of the string, throwing
>> StringIndexOutOfBoundsException when `charAt` is invoked (line 1740).
>>
>> We should check to make sure position does not exceed the string length
>> before deciding to check for a decimal symbol.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> move test to existing test file
LGTM. Thanks for the changes.
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19533#pullrequestreview-2099786378
Re: RFR: 8332750: Broken link in CatalogFeatures.html
On Tue, 4 Jun 2024 21:17:43 GMT, Joe Wang wrote: > Fix a broken link. Marked as reviewed by naoto (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19551#pullrequestreview-2097510057
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v8]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 26 commits: - Corrected comments - Merge branch 'master' into JDK-8332161-restoreEcho-Test - Check restored echo with stty -a - incorporated the same fix from 8332922 - Merge branch 'master' into JDK-8332161-restoreEcho-Test - Merge branch 'master' into JDK-8332161-restoreEcho-Test - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Added comment for restoreEchoLock - fixing typo - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - ... and 16 more: https://git.openjdk.org/jdk/compare/9686e804...1c0ab8eb - Changes: https://git.openjdk.org/jdk/pull/19315/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19315=07 Stats: 158 lines in 2 files changed: 158 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v4]
On Tue, 4 Jun 2024 09:07:44 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of DecimalFormat.format
Also it would be helpful to compare the performance without biased locking in
JDK11.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2148296971
Re: RFR: 8333462: Performance regression of new DecimalFormat() when compare to jdk11
On Tue, 4 Jun 2024 02:32:28 GMT, lingjun-cg wrote:
> Run the below benchmark test ,it show the average time of new
> DecimalFormat() increase 18% when compare to jdk 11.
>
> the result with jdk 11:
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
>
> the result with current jdk:
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> @Setup(Level.Trial)
> public void setup() {
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
> }
>
>
> Compare the flame graph it shows the
> java.text.DecimalFormatSymbols#findNonFormatChar takes a significant time.
> After replacing the lambda implementation with a simple loop , it shows
> nearly the same performance as jdk 11.
>
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> [flame-graph-jdk11-jdk21.zip](https://github.com/user-attachments/files/15541764/flame-graph-jdk11-jdk21.zip)
LGTM
src/java.base/share/classes/java/text/DecimalFormatSymbols.java line 866:
> 864: private char findNonFormatChar(String src, char defChar) {
> 865: for (int i = 0; i < src.length(); i++) {
> 866: if (Character.getType(src.charAt(i)) != Character.FORMAT) {
return of `src.charAt(i)` can be reused
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19534#pullrequestreview-2097161949
PR Review Comment: https://git.openjdk.org/jdk/pull/19534#discussion_r1626467416
Re: RFR: 8333396: Performance regression of DecimalFormat.format [v4]
On Tue, 4 Jun 2024 09:07:44 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of DecimalFormat.format
Thanks for splitting up the PR/JBS. For this one, I am still not sure why this
StringBuffer/Builder locking became an issue, as the code is basically the same
in JDK11 (meaning StringBuffer has always been used). Would it be possible if
the regression was caused by some other changes?
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2148184952
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v7]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Check restored echo with stty -a - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/2477adf4..c1ea5d59 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=05-06 Stats: 68 lines in 2 files changed: 8 ins; 38 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8333456: CompactNumberFormat integer parsing fails when string has no suffix
On Mon, 3 Jun 2024 22:32:54 GMT, Justin Lu wrote:
> Please review this PR which handles incorrect CompactNumberFormat integer
> only parsing when there is no suffix.
>
> See the following snippet,
>
>
> var fmt = NumberFormat.getCompactNumberInstance(Locale.US,
> NumberFormat.Style.SHORT)
> fmt.setParseIntegerOnly(true)
> fmt.parse("5K") // returns 5000
> fmt.parse("50.00") // returns 50
> fmt.parse("5") // unexpectedly throws StringIndexOutOfBoundsException
> fmt.parse("5 ") // returns 5
>
>
> Within the `parse` method, there is code that advances `position` past the
> fractional portion to find the suffix when `parseIntegerOnly()` is true.
> However, if a valid string input is given with no suffix,
> `DecimalFormat.subparseNumber()` will fully iterate through the string and
> `position` will be equal to the length of the string, throwing
> StringIndexOutOfBoundsException when `charAt` is invoked (line 1740).
>
> We should check to make sure position does not exceed the string length
> before deciding to check for a decimal symbol.
The fix looks good. For the test, can we add the test case into an existing
test, instead of creating a new test case file?
-
PR Review: https://git.openjdk.org/jdk/pull/19533#pullrequestreview-2096998510
Re: RFR: 8333086: Using Console.println is unnecessarily slow due to JLine initalization
On Thu, 30 May 2024 13:50:33 GMT, Jan Lahoda wrote:
> Consider these two programs:
>
>
> public class SystemPrint {
> public static void main(String... args) {
> System.err.println("Hello!");
> }
> }
>
> and:
>
> public class IOPrint {
> public static void main(String... args) {
> java.io.IO.println("Hello!");
> }
> }
>
>
> They do the same conceptual thing - write a text to the output. But,
> `IO.println` delegates to `Console.println`, which then delegates to a
> `Console` backend, and the default backend is currently based on JLine.
>
> The issues is that JLine takes a quite a long time to initialize, and in a
> program like this, JLine is not really needed - it is used to provide better
> editing experience when reading input, but there's no reading in these
> programs.
>
> For example, on my computer:
>
> $ time java -classpath /tmp SystemPrint
> Hello!
>
> real0m0,035s
> user0m0,019s
> sys 0m0,019s
>
> $ time java -classpath /tmp --enable-preview IOPrint
> Hello!
>
> real0m0,165s
> user0m0,324s
> sys 0m0,042s
>
>
> The proposal herein is to delegate to the simpler `Console` backend from
> `java.base` as long as the user only uses methods that print to output, and
> switch to the JLine delegate when other methods (typically input) is used.
> Note that while technically `writer()` is a method doing output, it will
> force JLine initialization to avoid possible problems if the client caches
> the writer and uses it after switching the delegates.
>
> With this patch, I can get timing like this:
>
> $ time java --enable-preview -classpath /tmp/ IOPrint
> Hello!
>
> real0m0,051s
> user0m0,038s
> sys 0m0,020s
>
>
> which seems much more acceptable.
>
> There is also #19467, which may reduce the time further.
>
> A future work might focus on making JLine initialize faster, but avoiding
> JLine initialization in case where we don't need it seems like a good step to
> me in any case.
Looks good with a minor nit.
test/jdk/jdk/internal/jline/LazyJdkConsoleProvider.java line 74:
> 72: ProcessBuilder builder =
> 73:
> ProcessTools.createTestJavaProcessBuilder("--enable-preview",
> 74:
> "-verbose:class",
Better to explicitly specify `-Djdk.console=jdk.internal.le`
-
PR Review: https://git.openjdk.org/jdk/pull/19479#pullrequestreview-2096855434
PR Review Comment: https://git.openjdk.org/jdk/pull/19479#discussion_r1626281874
Re: Instant.now(Clock) and InstantSource
+1 for not deprecating the method that takes Clock. It would give existing users unnecessary warnings. Naoto On 6/4/24 12:17 AM, Stephen Colebourne wrote: On Mon, 3 Jun 2024 at 22:25, Kurt Alfred Kluever wrote: It feels a bit strange that you can't pass an `InstantSource` to `Instant.now(...)`, but you _can_ pass a `Clock` (which of course has a "useless" `ZoneId` when creating an `Instant`). Therefore, I'd like to propose one of the following API changes: 1) adding `Instant.now(InstantSource)` 2) deprecating `Instant.now(Clock)` in favor of `clock.instant()` I have no problem with adding `Instant.now(InstantSource)`, but I think deprecating the `Clock` method is unnecessary (given it will never be removed AFAICT, and it is no doubt in widespread use). Stephen
Re: RFR: 8333396: Performance regression of new DecimalFormat and DecimalFormat.format [v2]
On Mon, 3 Jun 2024 06:13:35 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Performance regression of new DecimalFormat
>> After comparing the flame graph between current jdk and jdk 11, the method
>> java.text.DecimalFormatSymbols#findNonFormatChar takes a significant time.
>> The performance becomes as good as jdk11 after replacing it with a simple
>> loop implementation.
>>
>>
>>
>> ### Test result
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndForma...
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of new DecimalFormat and
> DecimalFormat.format
Hi,
Can you please provide more details? As to StringBuffer, I think it is being
used since those classes in `java.text` package have been created. I am not
sure why that contributes to what you described as the "performance regression".
Separately, please split this PR into two, as combining two different issues
into a single JBS issue/PR is not right. The second issue is likely due to
loading stream classes for the first time at JVM startup.
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2146264799
Integrated: 8333103: Re-examine the console provider loading
On Wed, 29 May 2024 19:51:36 GMT, Naoto Sato wrote: > There is an initialization code in `Console` class that searches for the > Console implementations. Refactoring the init code not to use lambda/stream > would reduce the (initial) number of loaded classes by about 100 for > java.base implementations. This would become relevant when the java.io.IO > (JEP 477) uses Console as the underlying framework. This pull request has now been integrated. Changeset: 9686e804 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/9686e804a2b058955ff88149c54a0a7896c0a2eb Stats: 23 lines in 1 file changed: 5 ins; 0 del; 18 mod 8333103: Re-examine the console provider loading Reviewed-by: redestad, jpai - PR: https://git.openjdk.org/jdk/pull/19467
Re: RFR: 8333103: Re-examine the console provider loading [v2]
On Thu, 30 May 2024 19:48:25 GMT, Naoto Sato wrote: >> There is an initialization code in `Console` class that searches for the >> Console implementations. Refactoring the init code not to use lambda/stream >> would reduce the (initial) number of loaded classes by about 100 for >> java.base implementations. This would become relevant when the java.io.IO >> (JEP 477) uses Console as the underlying framework. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Clarify the comment for multiple impls in the module case Thank you for your reviews! - PR Comment: https://git.openjdk.org/jdk/pull/19467#issuecomment-2145561992
Re: RFR: 8333103: Re-examine the console provider loading [v2]
> There is an initialization code in `Console` class that searches for the > Console implementations. Refactoring the init code not to use lambda/stream > would reduce the (initial) number of loaded classes by about 100 for > java.base implementations. This would become relevant when the java.io.IO > (JEP 477) uses Console as the underlying framework. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Clarify the comment for multiple impls in the module case - Changes: - all: https://git.openjdk.org/jdk/pull/19467/files - new: https://git.openjdk.org/jdk/pull/19467/files/ae06bb2c..42a45323 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19467=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19467=00-01 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19467.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19467/head:pull/19467 PR: https://git.openjdk.org/jdk/pull/19467
Re: RFR: 8333103: Re-examine the console provider loading
On Thu, 30 May 2024 18:38:59 GMT, Pavel Rappo wrote: >> In fact, this started simply for incorporating JLine implementation into >> Console, and using ServiceLoader was a mere means to load its impl as it >> resides outside the java.base module. So yes, module and its console >> implementation is 1:1, which is reflected by the system property >> `jdk.console` that takes the module name. So that `break;` effectively >> shortcut the unnecessary looping (I don't think it would happen btw). >> That said, I think it needs to be described in the comment above that piece >> of code. I will add it shortly. > > So, it's the previous (stream) version that was more permissive and > inadvertently so, yes? Yes, correct. If hypothetically Jline provided two impls, it were not specified which impl was used. Now we only use the first one found. - PR Review Comment: https://git.openjdk.org/jdk/pull/19467#discussion_r1621284108
Re: RFR: 8333103: Re-examine the console provider loading
On Thu, 30 May 2024 08:23:25 GMT, Pavel Rappo wrote: >> It's only semantically the same if we assume a module can only provide a >> single `JdkConsoleProvider`, no? The `break;` disallows multiple providers >> (for disjoint sets of charsets) in a single module. > > Claes has described the issue well. Like I said, `break` short-circuits the > search. If a module can provide more than one console provider, the suggested > stream-less replacement is **not** equivalent. > > If a module can provide more than one console provider, not only the code > needs to be fixed, but a relevant test should be added too. The test should > be in this PR, but tagged with the initial bug, [8295803], not this > (performance) bug. > > [8295803]: https://bugs.openjdk.org/browse/JDK-8295803 In fact, this started simply for incorporating JLine implementation into Console, and using ServiceLoader was a mere means to load its impl as it resides outside the java.base module. So yes, module and its console implementation is 1:1, which is reflected by the system property `jdk.console` that takes the module name. So that `break;` effectively shortcut the unnecessary looping (I don't think it would happen btw). That said, I think it needs to be described in the comment above that piece of code. I will add it shortly. - PR Review Comment: https://git.openjdk.org/jdk/pull/19467#discussion_r1621231072
Re: RFR: 8333103: Re-examine the console provider loading
On Wed, 29 May 2024 21:39:37 GMT, Pavel Rappo wrote: >> src/java.base/share/classes/java/io/Console.java line 673: >> >>> 671: return new ProxyingConsole(jc); >>> 672: } >>> 673: break; >> >> Suggestion: >> >> >> The original `findAny` is only after `filter(Objects::nonNull)` meaning we >> don't return `null` on a null `jcp.console` result. > > Yes, `break` guarantees that the search completes one way or another once the > module name has been matched. This is not how it used to be done. Right. Since `findAny` is after the module name matching, there is at most 1 match. In the case we didn't find any, the final `orElse(null)` eventually returns null. So the refactored code is semantically the same. - PR Review Comment: https://git.openjdk.org/jdk/pull/19467#discussion_r1619537359
RFR: 8333103: Re-examine the console provider loading
There is an initialization code in `Console` class that searches for the Console implementations. Refactoring the init code not to use lambda/stream would reduce the (initial) number of loaded classes by about 100 for java.base implementations. This would become relevant when the java.io.IO (JEP 477) uses Console as the underlying framework. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/19467/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19467=00 Issue: https://bugs.openjdk.org/browse/JDK-8333103 Stats: 20 lines in 1 file changed: 4 ins; 0 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/19467.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19467/head:pull/19467 PR: https://git.openjdk.org/jdk/pull/19467
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 18:50:38 GMT, Jonathan Gibbons wrote: >> src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java >> line 122: >> >>> 120: * see store.c of gennorm for more information and values >>> 121: */ >>> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50 */ >> >> This source file is coming from the upstream ICU4J project. Even if this is >> a `non-standard` comment, I would keep it as it is to minimize the merge >> effort. > > As a non-standard comment, it will trigger a warning (and hence an error), > since the prevailing standard for `java.base` is to compile with all warnings > enabled (`-Xlint`) and no warnings found (verified by `-Werror`) > > The alternative would be to use `@SuppressWarnings` for the > `dangling-doc-comment` warning, but that too would be a code change to these > imported files. OK, we will need to live with it. - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1617854422
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote: > With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). `GregorianCalendar` and ICU4J files LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19130#pullrequestreview-2083793439
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v6]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits: - incorporated the same fix from 8332922 - Merge branch 'master' into JDK-8332161-restoreEcho-Test - Merge branch 'master' into JDK-8332161-restoreEcho-Test - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Added comment for restoreEchoLock - fixing typo - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - leftover typo - get/setEcho() -> echo() - ... and 13 more: https://git.openjdk.org/jdk/compare/b8f2ec90...2477adf4 - Changes: https://git.openjdk.org/jdk/pull/19315/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19315=05 Stats: 192 lines in 2 files changed: 192 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]
On Fri, 24 May 2024 16:36:32 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated on 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. >> >> Updated on 5/18/2024 >> >> Withdraw changes to jaxp.properties. The original idea was to match >> jaxp-strict.properties with regard to the properties. However, that change >> impact the configuration process, resulting in tests that verify the process >> to fail. >> >> Updated on 5/23/2024 >> >> Provide a template `jaxp-strict.template` instead of a properties file. This >> template can be used to create custom configuration files. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > rename the template to jaxp-strict.properties.template LGTM test/jaxp/javax/xml/jaxp/unittest/common/config/ConfigFileTest.java line 41: > 39: * @run driver common.config.ConfigFileTest 0 // verifies jaxp.properties > 40: * @run driver common.config.ConfigFileTest 1 // verifies > jaxp-strict.properties.template > 41: * @summary verifies the default JAXP configuration file jaxp.properties > and Summary would read better after the @test tag - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2079802647 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1615343327
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v5]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits: - Merge branch 'master' into JDK-8332161-restoreEcho-Test - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Added comment for restoreEchoLock - fixing typo - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - leftover typo - get/setEcho() -> echo() - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Used a dedicate lock for restoreEcho - ... and 11 more: https://git.openjdk.org/jdk/compare/ebc520e8...36649fbd - Changes: https://git.openjdk.org/jdk/pull/19315/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19315=04 Stats: 192 lines in 2 files changed: 192 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v4]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. 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 20 additional commits since the last revision: - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Added comment for restoreEchoLock - fixing typo - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - leftover typo - get/setEcho() -> echo() - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Used a dedicate lock for restoreEcho - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - ... and 10 more: https://git.openjdk.org/jdk/compare/8d2a0559...7311487f - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/1cd88c61..7311487f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=02-03 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Integrated: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook
On Fri, 10 May 2024 21:55:26 GMT, Naoto Sato wrote: > Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. This pull request has now been integrated. Changeset: 236432db Author: Naoto Sato URL: https://git.openjdk.org/jdk/commit/236432dbdb9bab4aece54c2fea08f055e5dbf97e Stats: 18 lines in 1 file changed: 10 ins; 0 del; 8 mod 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook Reviewed-by: prappo, joehw, smarks - PR: https://git.openjdk.org/jdk/pull/19184
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v10]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added comment for restoreEchoLock - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/69ec27d6..d4b6e695 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=09 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=08-09 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v3]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. 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 seven additional commits since the last revision: - fixing typo - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - leftover typo - get/setEcho() -> echo() - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/ad4a4e47..1cd88c61 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=01-02 Stats: 14345 lines in 305 files changed: 9600 ins; 3407 del; 1338 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v9]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. 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 11 additional commits since the last revision: - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - Used a dedicate lock for restoreEcho - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - copyright year - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - get/setEcho() - Addresses a review comment - Replaced another unused exception with _ - Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - ... and 1 more: https://git.openjdk.org/jdk/compare/472fb9f3...69ec27d6 - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/f69f747a..69ec27d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=08 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=07-08 Stats: 14344 lines in 304 files changed: 9600 ins; 3407 del; 1337 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v8]
On Wed, 22 May 2024 21:06:11 GMT, Pavel Rappo wrote: > Okay, but can we call it a best-effort attempt to restore the echo state? I > guess, it is a judgement call. That would be fair, and exactly what I am aiming for, considering we can do nothing for the `halt()` case. - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2125768900
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote:
> With the advent of JEP 467, `///` comments may be treated as documentation
> comments, and may be subject to the recently new `javac` warning about
> "dangling doc comments" in unexpected places.
>
> In keeping with the policy to keep the `java.base` module free of all `javac`
> warnings, this patch proposes edits to existing uses of `///`.
>
> There are two dominant policies in the proposed changes.
> 1. A long horizontal line of `/` is replaced by `//-`
> 2. A long vertical series of lines beginning `///` is replaced by lines
> beginning `//|`.
>
> As with all style changes, I have also tried to honor local usage, for
> consistency.
>
> In one place, a pair of comments appeared to contain directives (`CLOVER:ON`,
> `CLOVER:OFF`). I investigated the use of such comments to determine that the
> exact form of the comment prefix was not significant. (Phew!)
>
>
> (This PR is informally blocked by JEP 467).
src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java
line 122:
> 120: * see store.c of gennorm for more information and values
> 121: */
> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50 */
This source file is coming from the upstream ICU4J project. Even if this is a
`non-standard` comment, I would keep it as it is to minimize the merge effort.
src/java.base/share/classes/jdk/internal/icu/lang/UCharacterDirection.java line
61:
> 59: {
> 60: }
> 61: // CLOVER:ON
Same here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1610588637
PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1610586405
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v8]
On Wed, 22 May 2024 18:35:40 GMT, Pavel Rappo wrote: > I might be confused, but what if the shutdown hook completes and then some > application thread enters `readPassword`. If that thread manages to turn off > echo before all other shutdown hooks complete, it might never execute > `finally`, hence a race. Yes, that is possible. However I would say that it is equally unlikely as an app installs a shutdown hook with readPassword(), so I think this is OK too. BTW, if an app issues `Runtime.halt()` while `readPassword()` is waiting, then those shutdown hooks aren't even executed, thus ends up in not restoring the echo without a race, which I think is OK as well. - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2125602570
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v8]
On Tue, 21 May 2024 22:51:14 GMT, Naoto Sato wrote: >> Making sure `restoreEcho` correctly reflects the state in the shutdown >> thread, which differs from the application's thread that issues the >> `readPassword()` method. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Used a dedicate lock for restoreEcho Hi Pavel, > If I read > [this](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#shutdown) > correctly, due to the mechanics of JVM exit, we simply don't know which > thread finishes first: a thread that calls `readPassword` or the shutdown > hook. IIUC, the thread that waits on `readPassword()` is not a shutdown hook, right? Then I think it is guaranteed that it is still waiting when all the shutdown hooks are executed. > If the shutdown hook finishes first, then a `readPassword` thread can corrupt > the state: unlike the shutdown hook, which JVM _normally has to wait to > complete_, the `readPassword` thread can be terminated at any moment. It > might as well be terminated before `finally` but after `echo(false)`, in > which case we end up with echo turned off. After the shutdown hooks finish, then the VM is terminated (https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#termination). In there: finally clauses are not executed; So I think the shutdown hook's restoreEcho has the final say, sans the situation if some app installs a shutdown hook with `readPassword` but I don't think we can guarantee that case, and I believe it is OK. - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2125369806
Re: RFR: 8332161: Test restoring echo in the Console implementation (java.base) [v2]
> This test intends to verify the behavior of JdkConsole for the java.base > module, wrt restoring the echo. The test assumes the internal methods that > sets/gets the echo status of the platform. 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 five additional commits since the last revision: - leftover typo - get/setEcho() -> echo() - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - Merge branch 'JDK-8332084-restoreEcho-readLock' into JDK-8332161-restoreEcho-Test - initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/19315/files - new: https://git.openjdk.org/jdk/pull/19315/files/58151807..ad4a4e47 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19315=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19315=00-01 Stats: 34602 lines in 391 files changed: 29274 ins; 4096 del; 1232 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v8]
On Tue, 21 May 2024 22:51:14 GMT, Naoto Sato wrote: >> Making sure `restoreEcho` correctly reflects the state in the shutdown >> thread, which differs from the application's thread that issues the >> `readPassword()` method. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Used a dedicate lock for restoreEcho Based on an internal discussion regarding Stuart's above comment, I changed the `restoreEcho` field back to the original. Instead of making it volatile, synchronizing it with a dedicated lock would ensure visibility and atomic status updates (no races). - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2123561717
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v8]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Used a dedicate lock for restoreEcho - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/e58fbdcd..f69f747a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=07 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=06-07 Stats: 66 lines in 3 files changed: 15 ins; 35 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
RFR: 8332161: Need a test for restoring echo in the Console implementation (java.base)
This test intends to verify the behavior of JdkConsole for the java.base module, wrt restoring the echo. The test assumes the internal methods that sets/gets the echo status of the platform. - Depends on: https://git.openjdk.org/jdk/pull/19184 Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/19315/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19315=00 Issue: https://bugs.openjdk.org/browse/JDK-8332161 Stats: 196 lines in 2 files changed: 196 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19315.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19315/head:pull/19315 PR: https://git.openjdk.org/jdk/pull/19315
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v7]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. 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 ten additional commits since the last revision: - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - copyright year - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - get/setEcho() - Addresses a review comment - Replaced another unused exception with _ - Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/82d30b3b..e58fbdcd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=06 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=05-06 Stats: 41521 lines in 532 files changed: 34514 ins; 5047 del; 1960 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
Re: RFR: 8331851: Add specific regression leap year tests for Calendar.roll()
On Wed, 15 May 2024 09:39:16 GMT, serhiysachkov wrote: > Add specific regression tests for Calendar.roll() method to explicitly > various leap year test scenarios. This is inspired by the ambiguity which > occurred in leap year unaware test creation as in case with Calendar.add() in > swing component test case as detailed in > (https://bugs.openjdk.org/browse/JDK-8327088). LGTM. I might want to remove those test case names as they are apparent from the parameters, but it's OK. - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19247#pullrequestreview-2064330497
Re: RFR: 8332424: Update IANA Language Subtag Registry to Version 2024-05-16
On Fri, 17 May 2024 18:04:02 GMT, Justin Lu wrote: > Please review this PR which is a trivial update to the IANA sub tag registry > to version 2024-05-16. Locale tests pass as expected after update. > > Associated announcement -> > https://mm.icann.org/pipermail/ietf-languages-announcements/2024-May/91.html LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19286#pullrequestreview-2064215561
Re: RFR: 8330276: Console methods with explicit Locale [v7]
On Wed, 15 May 2024 17:05:17 GMT, Naoto Sato wrote: >> Proposing new overloaded methods in `java.io.Console` class that explicitly >> take a `Locale` argument. Existing methods rely on the default locale, so >> the users won't be able to format their prompts/outputs in a certain locale >> explicitly. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reverted test workaround. Fixed JLine (backing out a questionable change) The changeset included a workaround patch to the upstream JLine. An issue for it was created by Jan: https://github.com/jline/jline3/issues/982 - PR Comment: https://git.openjdk.org/jdk/pull/18923#issuecomment-2115684085
Integrated: 8331202: Support for Duration until another Instant
On Mon, 29 Apr 2024 19:40:31 GMT, Naoto Sato wrote: > A new method on Instant to return the duration `until` another Instant is > suggested per the following discussion thread: > > https://mail.openjdk.org/pipermail/core-libs-dev/2024-April/122131.html > > A CSR has also been drafted. This pull request has now been integrated. Changeset: 25991516 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/259915168d6656b1b8ddad03c377082d6a5224e5 Stats: 58 lines in 2 files changed: 55 ins; 0 del; 3 mod 8331202: Support for Duration until another Instant Reviewed-by: joehw, scolebourne, rriggs - PR: https://git.openjdk.org/jdk/pull/19007
Re: RFR: 8331202: Support for Duration until another Instant [v3]
On Wed, 1 May 2024 17:21:20 GMT, Naoto Sato wrote: >> A new method on Instant to return the duration `until` another Instant is >> suggested per the following discussion thread: >> >> https://mail.openjdk.org/pipermail/core-libs-dev/2024-April/122131.html >> >> A CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addressing CSR reviews Hearing no further comments, integrating this PR. - PR Comment: https://git.openjdk.org/jdk/pull/19007#issuecomment-2115651450
Integrated: 8330276: Console methods with explicit Locale
On Tue, 23 Apr 2024 20:35:43 GMT, Naoto Sato wrote: > Proposing new overloaded methods in `java.io.Console` class that explicitly > take a `Locale` argument. Existing methods rely on the default locale, so the > users won't be able to format their prompts/outputs in a certain locale > explicitly. This pull request has now been integrated. Changeset: 7cff04fc Author: Naoto Sato URL: https://git.openjdk.org/jdk/commit/7cff04fc8a8114a297437aa526b18b6185831eac Stats: 441 lines in 8 files changed: 337 ins; 24 del; 80 mod 8330276: Console methods with explicit Locale Reviewed-by: joehw, rriggs, jlahoda - PR: https://git.openjdk.org/jdk/pull/18923
Re: RFR: 8330276: Console methods with explicit Locale [v7]
> Proposing new overloaded methods in `java.io.Console` class that explicitly > take a `Locale` argument. Existing methods rely on the default locale, so the > users won't be able to format their prompts/outputs in a certain locale > explicitly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reverted test workaround. Fixed JLine (backing out a questionable change) - Changes: - all: https://git.openjdk.org/jdk/pull/18923/files - new: https://git.openjdk.org/jdk/pull/18923/files/cace6171..7833da36 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18923=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18923=05-06 Stats: 9 lines in 2 files changed: 2 ins; 5 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923 PR: https://git.openjdk.org/jdk/pull/18923
Re: RFR: 8330276: Console methods with explicit Locale [v6]
> Proposing new overloaded methods in `java.io.Console` class that explicitly > take a `Locale` argument. Existing methods rely on the default locale, so the > users won't be able to format their prompts/outputs in a certain locale > explicitly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Adding workaround to the test due to JLine's questionable behavior - Changes: - all: https://git.openjdk.org/jdk/pull/18923/files - new: https://git.openjdk.org/jdk/pull/18923/files/6fd17574..cace6171 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18923=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18923=04-05 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923 PR: https://git.openjdk.org/jdk/pull/18923
Re: RFR: 8305457: Implement java.io.IO [v9]
On Mon, 13 May 2024 09:56:35 GMT, Pavel Rappo wrote: >> Please review this PR which introduces the `java.io.IO` top-level class and >> three methods to `java.io.Console` for [Implicitly Declared Classes and >> Instance Main Methods (Third Preview)]. >> >> This PR has been obtained as `git merge --squash` of a now obsolete [draft >> PR]. >> >> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: >> https://bugs.openjdk.org/browse/JDK-8323335 >> [draft PR]: https://github.com/openjdk/jdk/pull/18921 > > Pavel Rappo 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 17 additional > commits since the last revision: > > - Escape prompt > - Merge branch 'master' into 8305457-Implement-java.io.IO > - Clarify input charset > - Make IO final > - Fix System.console().readln(null) in jshell > >Without it, jshell hangs on me. Will think of a test. > - Fix typo > - Merge branch 'master' into 8305457-Implement-java.io.IO > - Simplify output.exp > - Cover null prompt in input tests > - Make input test parametric > - ... and 7 more: https://git.openjdk.org/jdk/compare/42e6a104...17100ab8 LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2056583806
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v6]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/e178f0de..82d30b3b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=05 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=04-05 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v5]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. 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 six additional commits since the last revision: - Merge branch 'master' into JDK-8332084-restoreEcho-readLock - get/setEcho() - Addresses a review comment - Replaced another unused exception with _ - Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/fb5beb11..e178f0de Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=04 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=03-04 Stats: 4029 lines in 159 files changed: 2537 ins; 817 del; 675 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v4]
On Mon, 13 May 2024 21:32:18 GMT, Naoto Sato wrote: >> Making sure `restoreEcho` correctly reflects the state in the shutdown >> thread, which differs from the application's thread that issues the >> `readPassword()` method. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addresses a review comment Thanks, Stuart. The shutdown hook in question is in the `java.base`'s JdkConsole implementation, so the save/restore responsibility is already pushed down to each provider. Thinking that `java.base`'s implementation is now non-default, and the possibility of the race condition is quite low, I now think making `restoreEcho` as volatile (the current PR) is the most practical solution, unless we could find a reasonably simple way to resolve the contention. BTW, the `java.base`'s shutdown hook has the special slot `0`, which is guaranteed to be called first: https://github.com/openjdk/jdk/blob/e91492ab4333c61f39b50eb428fa932131a5b908/src/java.base/share/classes/java/lang/Shutdown.java#L47 - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2110862963
Re: RFR: 8330276: Console methods with explicit Locale [v5]
> Proposing new overloaded methods in `java.io.Console` class that explicitly > take a `Locale` argument. Existing methods rely on the default locale, so the > users won't be able to format their prompts/outputs in a certain locale > explicitly. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge branch 'master' into JDK-8330276-Console-with-explicit-Locale - Removed JdkConsole.printf() - Addresses a CSR review comment - Merge branch 'master' into JDK-8330276-Console-with-explicit-Locale - Addressed review comments - Not using System.err - spacing - initial commit - Changes: https://git.openjdk.org/jdk/pull/18923/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18923=04 Stats: 438 lines in 7 files changed: 335 ins; 24 del; 79 mod Patch: https://git.openjdk.org/jdk/pull/18923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923 PR: https://git.openjdk.org/jdk/pull/18923
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v4]
On Mon, 13 May 2024 21:32:18 GMT, Naoto Sato wrote: >> Making sure `restoreEcho` correctly reflects the state in the shutdown >> thread, which differs from the application's thread that issues the >> `readPassword()` method. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addresses a review comment OK, I realized removing checking `restoreEcho` does not work, here is why. > It should be possible to provide something adequate based on atomic > primitives. However, it would complicate code. So, let's first clarify the > problem that `restoreEcho` is trying to solve: why is it important to restore > the echo state upon JVM exit? What would happen if we fail to do that? Can it > be demonstrated? `restoreEcho`, as I understand it, does what it stands, ie., restore the platform console's echo status. Since `java.base`'s Console implementation relies on the platform's echo function, JVM needs to restore the original echo state after JVM quits. So in an unlike situation where the echo is off when the JVM starts, my proposal won't work. We'll still have restoreEcho check in the shutdown thread. - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2110644430
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v4]
On Mon, 13 May 2024 21:32:18 GMT, Naoto Sato wrote: >> Making sure `restoreEcho` correctly reflects the state in the shutdown >> thread, which differs from the application's thread that issues the >> `readPassword()` method. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addresses a review comment I thought of the same scenario that is certainly possible. Now I am tempted to avoid this race condition altogether by removing checking restoreEcho and always issue echo(true). What do you guys think? - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2110258850
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]
On Mon, 13 May 2024 17:06:32 GMT, Naoto Sato wrote: >> Making sure `restoreEcho` correctly reflects the state in the shutdown >> thread, which differs from the application's thread that issues the >> `readPassword()` method. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Replaced another unused exception with _ Right, making the field volatile is a much safer solution. - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2108832766
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v4]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addresses a review comment - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/3e7d2e0c..fb5beb11 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=02-03 Stats: 8 lines in 1 file changed: 0 ins; 2 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
Re: In support of Instant.minus(Instant)
Hi, With Stephen/Roger's comments, as well as Kevin's observation that until(end) has a good argument ordering that is easy to understand, I'd still propose `until()`. Please post if you have further comments. Naoto On 5/3/24 6:39 AM, Roger Riggs wrote: Hi, I would also reinforce Stephen's early observation that the pattern for "until" methods in java.time includes those of the XXXDate classes, with a single Temporal parameter. Period and Duration are similar values holding relative TemporalAmounts. public Period until(ChronoLocalDate endDateExclusive) In addition to Instant, the LocalTime class might also benefit from adding: public Duration until(LocalTime endExclusive)` The API design of java.time included an emphasis on consistent naming across the packages. Regards, Roger On 5/2/24 4:01 PM, Naoto Sato wrote: `Temporal` interface is clear that its `minus` methods return objects of the same `Temporal` type, and `until` calculates the amount of time until another `Temporal` type. Introducing `Instant.minus` that returns `Duration` would be confusing to me. Naoto On 5/2/24 10:41 AM, Éamonn McManus wrote: I'd say too that this makes intuitive sense based on algebra. If we have: /instant1/ + /duration/ = /instant2/ then we can subtract /duration/ from both sides: /instant1 = instant2 - duration/ or we can subtract /instant1/ from both sides: /duration = instant2 - instant1/ There's no manipulation we can do that would cause us to try to add instants together, and it's a bit surprising for the API to allow the first subtraction but not the second. I also think that if I see instant2.minus(instant1) it's immediately obvious to me what that means, while instant1.until(instant2) seems both less discoverable and less obvious. On Thu, 2 May 2024 at 10:29, Louis Wasserman <mailto:lowas...@google.com>> wrote: That doesn't follow for me at all. The structure formed by Instants and Durations is an affine space <https://en.wikipedia.org/wiki/Affine_space#Definition>, with instants the points and durations the vectors. (An affine space is a vector space without a distinguished origin, which of course Instants don't have.) It is 100% standard to use the minus sign for the operation "point - point = vector," even when "point + point" is not defined, and to use all the other standard idioms for subtraction; the Wikipedia article uses "subtraction" and "difference" ubiquitously. Personally, I'd be willing to live with a different name for the operation, but consider "users keep getting it wrong" a strong enough argument all by itself for a version with the swapped argument order; it's not obvious to me that another API with the same argument order adds enough value over Duration.between to bother with. On Thu, May 2, 2024 at 10:04 AM Stephen Colebourne mailto:scolebou...@joda.org>> wrote: On Thu, 2 May 2024 at 15:58, Kurt Alfred Kluever mailto:k...@google.com>> wrote: > instant − instant = duration // what we're discussing > instant + duration = instant // satisfied by instant.plus(duration) > instant - duration = instant // satisfied by instant.minus(duration) > duration + duration = duration // satisfied by duration.plus(duration) > duration - duration = duration // satisfied by duration.minus(duration) > duration × real number = duration // satisfied by duration.multipliedBy(long) > duration ÷ real number = duration // satisfied by duration.dividedBy(long) > > All but the first operation have very clear translations from conceptual model to code. I'm hoping we can achieve the same clarity for instant - instant by using the obvious name: instant.minus(instant) But you can't have instant + instant = ??? It doesn't make sense. This is at the heart of why minus isn't right in this case. Stephen -- Louis Wasserman (he/they)
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]
On Mon, 13 May 2024 17:06:32 GMT, Naoto Sato wrote: >> Making sure `restoreEcho` correctly reflects the state in the shutdown >> thread, which differs from the application's thread that issues the >> `readPassword()` method. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Replaced another unused exception with _ Thanks, Pavel. > While it's hard to test this change (hence the `noreg-hard` label), the > `restoreEcho` functionality does not seem to be tested at all. Should we add > a straightforward test for it, perhaps separately? Filed: https://bugs.openjdk.org/browse/JDK-8332161 - PR Comment: https://git.openjdk.org/jdk/pull/19184#issuecomment-2108282547
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v3]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Replaced another unused exception with _ - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/9daf6997..3e7d2e0c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
Re: RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook [v2]
> Making sure `restoreEcho` correctly reflects the state in the shutdown > thread, which differs from the application's thread that issues the > `readPassword()` method. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/19184/files - new: https://git.openjdk.org/jdk/pull/19184/files/43f555e4..9daf6997 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19184=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19184=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
RFR: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook
Making sure `restoreEcho` correctly reflects the state in the shutdown thread, which differs from the application's thread that issues the `readPassword()` method. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/19184/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19184=00 Issue: https://bugs.openjdk.org/browse/JDK-8332084 Stats: 7 lines in 1 file changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/19184.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19184/head:pull/19184 PR: https://git.openjdk.org/jdk/pull/19184
Re: RFR: 8305457: Implement java.io.IO [v7]
On Fri, 10 May 2024 14:54:10 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java line 61:
>>
>>> 59: @Override
>>> 60: public JdkConsole println(Object obj) {
>>> 61: pw.println(obj);
>>
>> Are these `println(...)` and `print(...)` methods intentionally not using a
>> `writeLock` unlike the `readln(...)` and `readLine(...)` methods which do
>> use (read and write) locks?
>
> When implementing, I asked myself if I must use any of those monitors and
> decided that I don't have to. My reasoning is below.
>
> `readLock`:
>
> - is used inside the object that `Reader reader` is initialised with, and
>
> - it also guards fields such as `char[] rcb`, `boolean restoreEcho` and
> `boolean shutdownHookInstalled`.
>
> Since `println` and `print` don't call methods on `reader` or access the
> above fields, they don't require `readLock`.
>
> `writeLock`:
>
> - is used inside objects that `Writer out` and `PrintWriter pw` are
> initialised with, and
> - also in compound operations that first write and then immediately read. (I
> assume, it's so that no concurrent write could sneak in between writing and
> reading parts of such a compound.)
>
> `println` or `print` don't call methods on `out` and certainly don't do any
> write-read compounds. That said, they call methods on `pw`. But `pw` uses
> `writeLock` internally. So in that sense we're covered.
>
> One potential concern is a write-write compound in `print`:
>
>
> @Override
> public JdkConsole print(Object obj) {
> pw.print(obj);
> pw.flush(); // automatic flushing does not cover print
> return this;
> }
>
>
> I'm saying write-_write_, not write-_flush_, because as far as
> synchronisation is concerned, `pw.flush()` should behave the same as
> `pw.print("")`.
>
> While not using `writeLock` is not strictly correct, I believe the potential
> execution interleavings with other writes are benign. What's the worst that
> could happen? You flush more than you expected? Big deal!
>
> Since we exhausted all the reasons to use `writeLock`, I don't think we need
> one.
>
>
>
> Naoto has already reviewed this PR with only minor comments. While that
> increases my confidence in that the implementation is correct, it doesn't
> hurt to request re-review of this specific part: @naotoj, do you think I
> should use any of those monitors?
I think your investigation is correct. As to the write-write case, there
already is the same pattern in (`formatter` basically utilizes `pw` underneath)
public JdkConsole format(String fmt, Object ... args) {
formatter.format(fmt, args).flush();
return this;
}
So I think it is acceptable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596976499
Integrated: 8331866: Add warnings for locale data dependence
On Wed, 8 May 2024 20:21:50 GMT, Naoto Sato wrote: > In order to enlighten users to not depend on a particular version of the > locale data, putting a note into the JDK vs. CLDR version chart would be > appropriate. This pull request has now been integrated. Changeset: 65abf24f Author: Naoto Sato URL: https://git.openjdk.org/jdk/commit/65abf24fde6432fb386a616bbadc5689975c3bf4 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod 8331866: Add warnings for locale data dependence Reviewed-by: jlu, srl, joehw - PR: https://git.openjdk.org/jdk/pull/19145
Re: RFR: 8331646: Add specific regression leap year tests [v4]
On Thu, 9 May 2024 09:10:20 GMT, serhiysachkov wrote:
>> Calendar.add() tests that describe its behavior.
>
> serhiysachkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JDK-8331646 updating impl according to review requests
LGTM. Please fix the typo before integrate
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 116:
> 114: Arguments.of("testDateAddSubtractLeapYear", 29,
> FEBRUARY, 2024, 365, -365, DATE, 29, FEBRUARY, 2024),
> 115: Arguments.of("testWeekOfYearAddSubtractLeapYear", 29,
> FEBRUARY, 2024, 52, -52, WEEK_OF_YEAR, 29, FEBRUARY, 2024),
> 116: Arguments.of("testDateOfMonthAddSubtractLeapYear", 29,
> FEBRUARY, 2024, 31, -31, DAY_OF_MONTH, 29, FEBRUARY, 2024),
"DateOfMonth" -> "DayOfMonth"
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19082#pullrequestreview-2048948832
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1596061264
Re: RFR: 8331866: Add warnings for locale data dependence [v2]
On Thu, 9 May 2024 16:57:49 GMT, Joe Wang 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 three additional >> commits since the last revision: >> >> - Addresses review comment >> - Merge branch 'master' into JDK-8331866-localedata-dependence-warning >> - initial commit > > src/java.base/share/classes/java/util/spi/LocaleServiceProvider.java line 157: > >> 155: * Note that the CLDR locale data are subject to change. Users should >> not assume >> 156: * that the locale data remain the same across CLDR versions. >> Otherwise, unexpected >> 157: * incompatible behaviors may occur, such as an exception on parsing a >> date. > > For those wondering what has changed from one version to another or whether a > particular change might have an impact after reading this note, I wonder if > some kind of notes or links might be helpful. A column to the table below to > add some notes? I'm sure they can always look at the CLDR release notes. Just > wonder if there's anything specific worth noting or a link. IMHO, adding an extra column to describe what has changed in CLDR would seem impractical as the significance of the changes depends on each reader. Instead, I added a sentence to navigate readers to CLDR's releases page for the deltas. - PR Review Comment: https://git.openjdk.org/jdk/pull/19145#discussion_r1595794498
Re: RFR: 8331866: Add warnings for locale data dependence [v2]
> In order to enlighten users to not depend on a particular version of the > locale data, putting a note into the JDK vs. CLDR version chart would be > appropriate. 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 three additional commits since the last revision: - Addresses review comment - Merge branch 'master' into JDK-8331866-localedata-dependence-warning - initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/19145/files - new: https://git.openjdk.org/jdk/pull/19145/files/f4a680c1..b33fe543 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19145=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19145=00-01 Stats: 16883 lines in 280 files changed: 8496 ins; 5891 del; 2496 mod Patch: https://git.openjdk.org/jdk/pull/19145.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19145/head:pull/19145 PR: https://git.openjdk.org/jdk/pull/19145
Integrated: 8329691: Support `nonlikelyScript` parent locale inheritance
On Mon, 6 May 2024 17:53:56 GMT, Naoto Sato wrote: > This PR is to implement the `nonlikelyScript` feature that went into CLDR > version 45 for migration purposes. In its release note, it states > (https://cldr.unicode.org/index/downloads/cldr-45): > > Migration > Changes to parentLocales require upgrading implementations that use that > element. In particular, they need to support the new nonlikelyScript value, > and use the appropriate explicit inheritance for each type of inheritance. > The v44 list of locales that inherit directly from root is retained for this > release, but will disappear in the future. So implementations should move as > quickly as possible to support the new value > > For example in `Russian` locales fallback, its likely script is `Cyrl` > (Cyrillic). Thus Russian locales with non-likely script, such as 'ru-Latn' > (Russian in Latin script) should fallback directly to `root`, bypassing `ru` > (Russian). CLDR has explicit parent locales for this nonlikely scripts, such > as `zh-Hant` -> `root` already, but the release note suggests this will go > away, and JDK needs to logically handle these non-likely script inheritance > cases. > > To implement this behavior, CLDRConverter build tool now generates the > `LocaleDataMetaInfo` for java.base module with the new `likelyScriptMap`, > which maps the script to its likely languages. Since the map is big, it is > lazily initialized when needed. The map is used at runtime to determine the > parent locale fallback based on implicit/explicit nonlikely Script > inheritance. This pull request has now been integrated. Changeset: c7d98df2 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/c7d98df2ac509ebc8f4e801a0874a9497c54c602 Stats: 287 lines in 5 files changed: 211 ins; 31 del; 45 mod 8329691: Support `nonlikelyScript` parent locale inheritance Reviewed-by: joehw - PR: https://git.openjdk.org/jdk/pull/19108
Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4) [v3]
On Wed, 8 May 2024 11:36:07 GMT, serhiysachkov wrote:
>> Calendar.add() tests that describe its behavior.
>
> serhiysachkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JDK-8331646 consolidating test methods into ParameterizedTests
Thanks for the changes. Also, match this PR title to the JBS title (remove that
`Task - P4`)
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 41:
> 39:
> 40: import static java.util.Calendar.*;
> 41: import static java.util.Calendar.MONTH;
It's better to explicitly declare which fields are statically imported from
`Calendar`.
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 54:
> 52: int value, int calendarField, int
> expectedDate, int expectedMonth,
> 53: int expectedYear) {
> 54: Calendar calendar = Calendar.getInstance();
Instead of using a factory on `Calendar`, this should explicitly construct a
`GregorianCalendar` instance which guarantees the leap-year behavior.
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 69:
> 67: public void testMonthYearAddSubtractNonLeapYear() {
> 68: Calendar calendar = Calendar.getInstance();
> 69: calendar.set(2024, 1, 29, 15, 0, 0);
It's easier to understand using `FEBRUARY`, instead of the immediate value `1`.
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 72:
> 70: calendar.add(Calendar.MONTH, 1);
> 71: calendar.add(YEAR, -1);
> 72: calendar.add(Calendar.MONTH, -1);
`Calendar.MONTH` -> `MONTH`
test/jdk/java/util/Calendar/CalendarLeapYearAddTest.java line 118:
> 116: Arguments.of("testMonthAddLeapYear", 29, 1, 2024, 1,
> MONTH, 29, MARCH, 2024),
> 117: Arguments.of("testOneMonthSubtractLeapYear", 31, 2,
> 2024, -1, MONTH, 29, FEBRUARY, 2024),
> 118: Arguments.of("testTwoMonthSubtractLeapYear", 30, 3,
> 2024, -2, MONTH, 29, FEBRUARY, 2024)
Please use those month fields in those arguments.
-
PR Review: https://git.openjdk.org/jdk/pull/19082#pullrequestreview-2046786753
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594729131
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594732962
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594730699
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594731131
PR Review Comment: https://git.openjdk.org/jdk/pull/19082#discussion_r1594742243
RFR: 8331866: Add warnings for locale data dependence
In order to enlighten users to not depend on a particular version of the locale data, putting a note into the JDK vs. CLDR version chart would be appropriate. - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/19145/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19145=00 Issue: https://bugs.openjdk.org/browse/JDK-8331866 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19145.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19145/head:pull/19145 PR: https://git.openjdk.org/jdk/pull/19145
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant constructor LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19140#pullrequestreview-2046346164
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant constructor Thanks for fixing this! src/java.base/share/classes/sun/util/locale/BaseLocale.java line 96: > 94: // Interned BaseLocale cache > 95: private static final ReferencedKeySet CACHE = > 96: ReferencedKeySet.create(true, > ReferencedKeySet.concurrentHashMapSupplier()); Should this supplier be in `BaseLocale` class? Otherwise `ReferencedKeySet` may end up with static suppliers for each map type? - PR Review: https://git.openjdk.org/jdk/pull/19140#pullrequestreview-2046313019 PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594429353
Re: RFR: 8330276: Console methods with explicit Locale [v4]
> Proposing new overloaded methods in `java.io.Console` class that explicitly > take a `Locale` argument. Existing methods rely on the default locale, so the > users won't be able to format their prompts/outputs in a certain locale > explicitly. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Removed JdkConsole.printf() - Changes: - all: https://git.openjdk.org/jdk/pull/18923/files - new: https://git.openjdk.org/jdk/pull/18923/files/29ff6063..2e83fc7b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18923=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18923=02-03 Stats: 20 lines in 5 files changed: 0 ins; 19 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923 PR: https://git.openjdk.org/jdk/pull/18923
Re: RFR: 8305457: Implement java.io.IO
On Tue, 7 May 2024 17:15:43 GMT, Pavel Rappo wrote:
>> test/jdk/java/io/IO/IO.java line 99:
>>
>>> 97: System.getProperty("test.jdk") + "/bin/java",
>>> 98: "--enable-preview",
>>> 99: "-Djdk.console=gibberish",
>>
>> The test comment suggests this test is testing the default console
>> implementation, but the invocation specifies `-Djdk.console=gibberish` which
>> defaults to java.base. Is this what you intended?
>
> That comment says that this test tests jdk.internal.io.JdkConsoleImpl, which
> belongs to java.base. But, if you read it the way you described, I should
> definitely rephrase it.
Sorry, I read it wrong. Your comment is clear so no need for rewording
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592840539
Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4) [v2]
On Tue, 7 May 2024 09:44:09 GMT, serhiysachkov wrote: >> Calendar.add() tests that describe its behavior. > > serhiysachkov has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8331646 updating to ParameterizedTests according to review request Sorry if I was unclear, but I meant to consolidate the test methods, as they are duplicates other than the calendar values. Those values should be parametrized and consolidate the test body into a single method. - PR Comment: https://git.openjdk.org/jdk/pull/19082#issuecomment-2098881008
Re: RFR: 8305457: Implement java.io.IO
On Mon, 6 May 2024 21:45:12 GMT, Pavel Rappo wrote:
> Please review this PR which introduces the `java.io.IO` top-level class and
> three methods to `java.io.Console` for [Implicitly Declared Classes and
> Instance Main Methods (Third Preview)].
>
> This PR has been obtained as `git merge --squash` of a now obsolete [draft
> PR].
>
> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]:
> https://bugs.openjdk.org/browse/JDK-8323335
> [draft PR]: https://github.com/openjdk/jdk/pull/18921
Looks good overall. Left some minor comments.
src/java.base/share/classes/java/io/Console.java line 189:
> 187: /**
> 188: * Writes a prompt as if by calling {@code print}, then reads a
> single line
> 189: * of text from this system console.
Nit: I would not add `system` here as `this console` is consistent with other
locations.
src/java.base/share/classes/java/io/ProxyingConsole.java line 107:
> 105: * {@inheritDoc}
> 106: *
> 107: * @throws IOError {@inheritDoc}
Probably I am missing something, but I see `Console` declares this throws
clause. Do we need this inheritDoc here?
test/jdk/java/io/IO/IO.java line 99:
> 97: System.getProperty("test.jdk") + "/bin/java",
> 98: "--enable-preview",
> 99: "-Djdk.console=gibberish",
The test comment suggests this test is testing the default console
implementation, but the invocation specifies `-Djdk.console=gibberish` which
defaults to java.base. Is this what you intended?
test/jdk/java/io/IO/Input.java line 33:
> 31: System.out.print(readln("?"));
> 32: }
> 33: }
Needs a newline
-
PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2041904750
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1591704961
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592764263
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592773917
PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1592776068
Re: RFR: 8330276: Console methods with explicit Locale [v3]
> Proposing new overloaded methods in `java.io.Console` class that explicitly > take a `Locale` argument. Existing methods rely on the default locale, so the > users won't be able to format their prompts/outputs in a certain locale > explicitly. 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 six additional commits since the last revision: - Addresses a CSR review comment - Merge branch 'master' into JDK-8330276-Console-with-explicit-Locale - Addressed review comments - Not using System.err - spacing - initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/18923/files - new: https://git.openjdk.org/jdk/pull/18923/files/7884cfe3..29ff6063 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18923=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18923=01-02 Stats: 33162 lines in 1907 files changed: 14282 ins; 12249 del; 6631 mod Patch: https://git.openjdk.org/jdk/pull/18923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923 PR: https://git.openjdk.org/jdk/pull/18923
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v3]
On Mon, 6 May 2024 17:52:07 GMT, Justin Lu wrote:
>> Please review this PR which corrects an edge case bug for
>> java.text.DecimalFormat that causes incorrect parsing results for strings
>> with very large exponent values.
>>
>> When parsing values with large exponents, if the value of the exponent
>> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value
>> of the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
>> mantissa. Both results are confusing and incorrect.
>>
>> For example,
>>
>>
>> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
>> fmt.parse(".1E2147483648"); // returns 0.0
>> fmt.parse(".1E9223372036854775808"); // returns 0.1
>> // For comparison
>> Double.parseDouble(".1E2147483648"); // returns Infinity
>> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>>
>>
>> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address PP for exp > Long.MAX_VALUE + more exp test cases
test/jdk/java/text/Format/DecimalFormat/LargeExponentsTest.java line 57:
> 55: @ParameterizedTest
> 56: @MethodSource({"largeExponentValues", "smallExponentValues",
> "bugReportValues", "edgeCases"})
> 57: public void overflowTest(String parseString, Double expectedValue) {
Since this is a regression test, it may be better having both 1-arg parse() and
2-arg parse() tested separately, instead of replacing.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1591405570
Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4)
On Fri, 3 May 2024 10:31:14 GMT, serhiysachkov wrote: > Calendar.add() tests that describe its behavior. I would prefer those test values are parametrized. - PR Review: https://git.openjdk.org/jdk/pull/19082#pullrequestreview-2041403213
RFR: 8329691: Support `nonlikelyScript` parent locale inheritance
This PR is to implement the `nonlikelyScript` feature that went into CLDR version 45 for migration purposes. In its release note, it states (https://cldr.unicode.org/index/downloads/cldr-45): Migration Changes to parentLocales require upgrading implementations that use that element. In particular, they need to support the new nonlikelyScript value, and use the appropriate explicit inheritance for each type of inheritance. The v44 list of locales that inherit directly from root is retained for this release, but will disappear in the future. So implementations should move as quickly as possible to support the new value For example in `Russian` locales fallback, its likely script is `Cyrl` (Cyrillic). Thus Russian locales with non-likely script, such as 'ru-Latn' (Russian in Latin script) should fallback directly to `root`, bypassing `ru` (Russian). CLDR has explicit parent locales for this nonlikely scripts, such as `zh-Hant` -> `root` already, but the release note suggests this will go away, and JDK needs to logically handle these non-likely script inheritance cases. To implement this behavior, CLDRConverter build tool now generates the `LocaleDataMetaInfo` for java.base module with the new `likelyScriptMap`, which maps the script to its likely languages. Since the map is big, it is lazily initialized when needed. The map is used at runtime to determine the parent locale fallback based on implicit/explicit nonlikely Script inheritance. - Commit messages: - cleanup - added tests - static AVAILABLE_LOCALES - initial commit Changes: https://git.openjdk.org/jdk/pull/19108/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19108=00 Issue: https://bugs.openjdk.org/browse/JDK-8329691 Stats: 287 lines in 5 files changed: 211 ins; 31 del; 45 mod Patch: https://git.openjdk.org/jdk/pull/19108.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19108/head:pull/19108 PR: https://git.openjdk.org/jdk/pull/19108
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v3]
On Mon, 6 May 2024 16:09:19 GMT, Jan Lahoda wrote:
>> When JLine reads a line, there may be a prompt provided. However, JLine will
>> not interpret the prompt literally, it will handle `%` specially. As a
>> consequence, doing:
>>
>> System.console().readLine("%%s");
>>
>>
>> will not print `%s`, as first `String.format` is used, which will convert
>> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
>> is to duplicate the `%`, so that JLine will print it.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains eight commits:
>
> - Adjusting test as suggested.
> - Merge branch 'master' into JDK-8331535
> - 8331535: Incorrect prompt for Console.readLine
> - Fixing test.
> - Attempting to stabilize the test.
> - Improving test to really test the redirect while stdin is connected to a
> terminal.
> - Fixing typo.
> - 8330998: System.console() writes to stderr when stdout is redirected
Looks good
test/jdk/java/io/Console/ConsolePromptTest.java line 29:
> 27: * @summary Verify the java.base's console provider handles the prompt
> correctly.
> 28: * @library /test/lib
> 29: * @run main/othervm --limit-modules java.base ConsolePromptTest
It would be helpful if we do another run with `-Djdk.console=java.base` too.
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19081#pullrequestreview-2041252866
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591300205
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Mon, 6 May 2024 16:05:58 GMT, Jan Lahoda wrote: >>> We have a test that checks if `System.console()` returns the correct >>> Console (or null) from the expected module >>> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >>> >> >> Good; then here we should indeed specify `-Djdk.console=jdk.internal.le`. >> Initially, I suggested an alternative wording (i.e. "default"), but that's >> inappropriate. After all, the test in question resides in >> `test/jdk/jdk/internal/jline`. So it only makes sense that it tests that >> concrete implementation. >> >>> Yeah, that would be helpful. >>> >> >> I filed a bug: https://bugs.openjdk.org/browse/JDK-8331681 > > Thanks. I've added `-Djdk.console=jdk.internal.le`, and also added a test for > the `java.base`'s console: `test/jdk/java/io/Console/ConsolePromptTest.java`. > I can easily remove the latter if this is not a reasonable form. Or I can fix > it as needed. Please let me know. > > Thanks. For the latter, since you are already at it, I just reassigned the bug to you - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591302360
Re: RFR: 8331535: Incorrect prompt for Console.readLine
On Fri, 3 May 2024 18:52:12 GMT, Pavel Rappo wrote: > Ideally, we should have separate tests that make sure that jdk.internal.le is > the default impl. We have a test that checks if `System.console()` returns the correct Console (or null) from the expected module (`test/jdk/java/io/Console/ModuleSelectionTest.java`) > We should also test this behaviour (i.e. % interpretation) on jdk.internal.io > Console impl. Yeah, that would be helpful. - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589629655
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]
On Fri, 3 May 2024 18:29:23 GMT, Justin Lu wrote:
>> Please review this PR which corrects an edge case bug for
>> java.text.DecimalFormat that causes incorrect parsing results for strings
>> with very large exponent values.
>>
>> When parsing values with large exponents, if the value of the exponent
>> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value
>> of the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
>> mantissa. Both results are confusing and incorrect.
>>
>> For example,
>>
>>
>> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
>> fmt.parse(".1E2147483648"); // returns 0.0
>> fmt.parse(".1E9223372036854775808"); // returns 0.1
>> // For comparison
>> Double.parseDouble(".1E2147483648"); // returns Infinity
>> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>>
>>
>> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
>
> Justin Lu has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - correct other test comment
> - reflect review
Marked as reviewed by naoto (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/19075#pullrequestreview-2038805949
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent
On Fri, 3 May 2024 08:47:03 GMT, Justin Lu wrote:
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
Looks good. Left some minor comments.
src/java.base/share/classes/java/text/DecimalFormat.java line 2590:
> 2588:
> 2589: if (subparse(text, pos, "",
> symbols.getMinusSignText(), exponentDigits, true, stat)) {
> 2590: // We parse the exponent with isExponent true,
> thus fitsIntoLong
Nit: `==` or `being` between `isExponent` and `true` may read better.
src/java.base/share/classes/java/text/DecimalFormat.java line 2596:
> 2594: exponent = -exponent;
> 2595: }
> 2596: sawExponent = true;
I see you removed this assignment. I am wondering if we need this variable at
all.
test/jdk/java/text/Format/DecimalFormat/LargeExponentsTest.java line 128:
> 126: // Trailing zeroes
> 127: Arguments.of("1.23E123", 1.23E123),
> 128: // Trailing zeroes - Past Long.MAX_VALUE length
Leading zeroes?
-
PR Review: https://git.openjdk.org/jdk/pull/19075#pullrequestreview-2038744656
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589560835
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589558570
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589573332
Re: RFR: 8331535: Incorrect prompt for Console.readLine
On Fri, 3 May 2024 11:12:48 GMT, Pavel Rappo wrote:
>> When JLine reads a line, there may be a prompt provided. However, JLine will
>> not interpret the prompt literally, it will handle `%` specially. As a
>> consequence, doing:
>>
>> System.console().readLine("%%s");
>>
>>
>> will not print `%s`, as first `String.format` is used, which will convert
>> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
>> is to duplicate the `%`, so that JLine will print it.
>
> test/jdk/jdk/internal/jline/JLineConsoleProviderTest.java line 27:
>
>> 25: * @test
>> 26: * @bug 8331535
>> 27: * @summary Verify the jdk.internal.le's console provider works properly.
>
> There's a hidden assumption in this test below that the _default_ console is
> `jdk.internal.le`. Is there any way to assert that?
Or maybe we could explicitly specify `-Djdk.console=jdk.internal.le` to the
test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r158945
Integrated: 8331582: Incorrect default Console provider comment
On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato wrote: > Fixing a comment in `java.io.Console`, which was a leftover from the fix to > https://bugs.openjdk.org/browse/JDK-8308591 This pull request has now been integrated. Changeset: cf2c80e4 Author: Naoto Sato URL: https://git.openjdk.org/jdk/commit/cf2c80e4fcd74b9d1d60e2358e7883bdd8a4ac80 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8331582: Incorrect default Console provider comment Reviewed-by: joehw, jlahoda, jlu, prappo - PR: https://git.openjdk.org/jdk/pull/19070
Re: RFR: 8331582: Incorrect default Console provider comment
On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato wrote: > Fixing a comment in `java.io.Console`, which was a leftover from the fix to > https://bugs.openjdk.org/browse/JDK-8308591 Thank you for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/19070#issuecomment-2093318995
Re: RFR: 8331582: Incorrect default Console provider comment [v2]
> Fixing a comment in `java.io.Console`, which was a leftover from the fix to > https://bugs.openjdk.org/browse/JDK-8308591 Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/io/Console.java Thanks. That reads better Co-authored-by: Pavel Rappo <32523691+pavelra...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/19070/files - new: https://git.openjdk.org/jdk/pull/19070/files/2ab4fdf9..ea465ee2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19070=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19070=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19070.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19070/head:pull/19070 PR: https://git.openjdk.org/jdk/pull/19070
RFR: 8331582: Incorrect default Console provider comment
Fixing a comment in `java.io.Console`, which was a leftover from the fix to https://bugs.openjdk.org/browse/JDK-8308591 - Commit messages: - initial commit Changes: https://git.openjdk.org/jdk/pull/19070/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19070=00 Issue: https://bugs.openjdk.org/browse/JDK-8331582 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19070.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19070/head:pull/19070 PR: https://git.openjdk.org/jdk/pull/19070
