8330276: Console methods with explicit Locale #18923

naotoj · 2024-04-23T20:35:43Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8330536 to be approved

Issues

JDK-8330276: Console methods with explicit Locale (Enhancement - P4)
JDK-8330536: Console methods with explicit Locale (CSR)

Reviewers

Joe Wang (@JoeWang-Java - Reviewer) ⚠️ Review applies to 7884cfe3
Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to 7884cfe3
Jan Lahoda (@lahodaj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18923/head:pull/18923
$ git checkout pull/18923

Update a local copy of the PR:
$ git checkout pull/18923
$ git pull https://git.openjdk.org/jdk.git pull/18923/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18923

View PR using the GUI difftool:
$ git pr show -t 18923

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18923.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-23T20:36:13Z

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-23T20:37:12Z

@naotoj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8330276: Console methods with explicit Locale

Reviewed-by: joehw, rriggs, jlahoda

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 19 new commits pushed to the master branch:

491b3b4: 8332256: Shenandoah: Do not visit heap threads during shutdown
9c02c8d: 8332255: Shenandoah: Remove duplicate definition of init mark closure
42ccb74: 8331940: ClassFile API ArrayIndexOutOfBoundsException with certain class files
61aff6d: 8332112: Update nsk.share.Log to don't print summary during VM shutdown hook
30bb066: 8332003: Clarify javadoc for MemoryLayout::offsetHandle
c4867c6: 8329273: C2 SuperWord: Some basic MemorySegment IR tests
8032d64: 8332245: C2: missing record_for_ign() call in GraphKit::must_be_not_null()
fa043ae: 8294880: Review running time of jdk/internal/shellsupport/doc/JavadocHelperTest.java
a5005c8: 8330814: Cleanups for KeepAliveCache tests
1a94447: 8332111: [BACKOUT] A way to align already compiled methods with compiler directives
... and 9 more: https://git.openjdk.org/jdk/compare/95a601316de06b4b0fbf6e3c7777be5d2a1ca978...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-23T20:37:50Z

@naotoj The following labels will be automatically applied to this pull request:

core-libs
kulla

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-04-24T17:20:41Z

Webrevs

JoeWang-Java · 2024-04-29T21:54:55Z

src/java.base/share/classes/java/io/Console.java

+     * the specified format string and arguments with the specified
+     * {@code locale}.
+     *
+     * @param  locale locale used for formatting


Specify the behavior when locale is null?

Good point. Brought the same wording from String.format

JoeWang-Java · 2024-04-29T21:57:22Z

src/java.base/share/classes/java/io/Console.java

+     *         Arguments referenced by the format specifiers in the format
+     *         string.  If there are more arguments than format specifiers, the
+     *         extra arguments are ignored.  The number of arguments is
+     *         variable and may be zero.  The maximum number of arguments is


readLine and readPassword don't have this statement (The number of arguments is variable and may be zero). Is this statement helpful for those methods as well?

Modified each description for args consistently.

naotoj · 2024-04-29T23:19:46Z

Thanks, Joe. Addressed your points (and updated the test case accordingly)

JoeWang-Java

Thanks for the update. Changes look good to me.

RogerRiggs · 2024-05-01T17:10:53Z

src/java.base/share/classes/java/io/ProxyingConsole.java

@@ -83,9 +85,17 @@ public Reader reader() {
     * {@inheritDoc}
     */
    @Override
-    public Console format(String fmt, Object ... args) {
+    public Console format(String format, Object ... args) {
+        return format(Locale.getDefault(Locale.Category.FORMAT), format, args);


Given the number of calls to Locale.getDefault(Locale.Category.FORMAT) it might be worthwhile to cache that in the Proxying Console or in the JdkConsoleImpl.

Initially, I thought about it but did not cache it here because it is cached in the Locale class, and the call returns the cached value.

RogerRiggs

lgtm

naotoj · 2024-05-14T15:59:59Z

/integrate

openjdk · 2024-05-14T16:00:48Z

@naotoj Since your change was applied there have been 86 commits pushed to the master branch:

95a6013: 8332042: Move MEMFLAGS to its own include file
5a4415a: 8331858: [nmt] VM.native_memory statistics should work in summary mode
4ba7447: 8326957: Implement JEP 474: ZGC: Generational Mode by Default
7ce4a13: 8332130: RISC-V: remove wrong instructions of Vector Crypto Extension
ea5eb74: 8326404: Assertion error when trying to compile switch with fallthrough with pattern
beea530: 8331907: BigInteger and BigDecimal should use optimized division
440782e: 8331466: Problemlist serviceability/dcmd/gc/RunFinalizationTest.java on generic-all
5ded8da: 8332085: Remove 10 year old transition check in GenerateCurrencyData tool
7c2c24f: 8261433: Better pkcs11 performance for libpkcs11:C_EncryptInit/libpkcs11:C_DecryptInit
ff4bf1c: 8332102: Add @since to package-info of jdk.security.jarsigner
... and 76 more: https://git.openjdk.org/jdk/compare/a8b3f194e811eed6b20bce71c752705c7cd50d24...master

It was not possible to rebase your changes automatically. Please merge master into your branch and try again.

…nge)

lahodaj

Looks good to me.

naotoj · 2024-05-15T19:27:06Z

/integrate

openjdk · 2024-05-15T19:28:26Z

Going to push as commit 7cff04f.
Since your change was applied there have been 20 commits pushed to the master branch:

8a4315f: 8331987: Enhance stacktrace clarity for CompletableFuture CancellationException
491b3b4: 8332256: Shenandoah: Do not visit heap threads during shutdown
9c02c8d: 8332255: Shenandoah: Remove duplicate definition of init mark closure
42ccb74: 8331940: ClassFile API ArrayIndexOutOfBoundsException with certain class files
61aff6d: 8332112: Update nsk.share.Log to don't print summary during VM shutdown hook
30bb066: 8332003: Clarify javadoc for MemoryLayout::offsetHandle
c4867c6: 8329273: C2 SuperWord: Some basic MemorySegment IR tests
8032d64: 8332245: C2: missing record_for_ign() call in GraphKit::must_be_not_null()
fa043ae: 8294880: Review running time of jdk/internal/shellsupport/doc/JavadocHelperTest.java
a5005c8: 8330814: Cleanups for KeepAliveCache tests
... and 10 more: https://git.openjdk.org/jdk/compare/95a601316de06b4b0fbf6e3c7777be5d2a1ca978...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-15T19:28:32Z

@naotoj Pushed as commit 7cff04f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

naotoj · 2024-05-16T16:27:25Z

The changeset included a workaround patch to the upstream JLine. An issue for it was created by Jan: jline/jline3#982

initial commit

598d0a1

openjdk bot added the csr Pull request needs approved CSR before integration label Apr 23, 2024

openjdk bot added core-libs core-libs-dev@openjdk.org kulla kulla-dev@openjdk.org labels Apr 23, 2024

naotoj added 2 commits April 23, 2024 13:48

spacing

6239b3c

Not using System.err

f3f879e

naotoj marked this pull request as ready for review April 24, 2024 17:17

openjdk bot added the rfr Pull request is ready for review label Apr 24, 2024

JoeWang-Java reviewed Apr 29, 2024

View reviewed changes

Addressed review comments

7884cfe

JoeWang-Java approved these changes Apr 30, 2024

View reviewed changes

RogerRiggs reviewed May 1, 2024

View reviewed changes

RogerRiggs approved these changes May 3, 2024

View reviewed changes

naotoj added 2 commits May 6, 2024 12:27

Merge branch 'master' into JDK-8330276-Console-with-explicit-Locale

a4317f0

Addresses a CSR review comment

29ff606

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels May 6, 2024

Removed JdkConsole.printf()

2e83fc7

naotoj added 3 commits May 14, 2024 09:08

Merge branch 'master' into JDK-8330276-Console-with-explicit-Locale

6fd1757

Adding workaround to the test due to JLine's questionable behavior

cace617

Reverted test workaround. Fixed JLine (backing out a questionable cha…

7833da3

…nge)

lahodaj approved these changes May 15, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 15, 2024

openjdk bot closed this May 15, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8330276: Console methods with explicit Locale #18923

8330276: Console methods with explicit Locale #18923

naotoj commented Apr 23, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 23, 2024

openjdk bot commented Apr 23, 2024 •

edited

Loading

openjdk bot commented Apr 23, 2024

mlbridge bot commented Apr 24, 2024 •

edited

Loading

JoeWang-Java Apr 29, 2024

naotoj Apr 29, 2024

JoeWang-Java Apr 29, 2024

naotoj Apr 29, 2024

naotoj commented Apr 29, 2024

JoeWang-Java left a comment

RogerRiggs May 1, 2024

naotoj May 1, 2024

RogerRiggs left a comment

naotoj commented May 14, 2024

openjdk bot commented May 14, 2024

lahodaj left a comment

naotoj commented May 15, 2024

openjdk bot commented May 15, 2024

openjdk bot commented May 15, 2024

naotoj commented May 16, 2024

8330276: Console methods with explicit Locale #18923

8330276: Console methods with explicit Locale #18923

Conversation

naotoj commented Apr 23, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 23, 2024

openjdk bot commented Apr 23, 2024 • edited Loading

openjdk bot commented Apr 23, 2024

mlbridge bot commented Apr 24, 2024 • edited Loading

Webrevs

JoeWang-Java Apr 29, 2024

Choose a reason for hiding this comment

naotoj Apr 29, 2024

Choose a reason for hiding this comment

JoeWang-Java Apr 29, 2024

Choose a reason for hiding this comment

naotoj Apr 29, 2024

Choose a reason for hiding this comment

naotoj commented Apr 29, 2024

JoeWang-Java left a comment

Choose a reason for hiding this comment

RogerRiggs May 1, 2024

Choose a reason for hiding this comment

naotoj May 1, 2024

Choose a reason for hiding this comment

RogerRiggs left a comment

Choose a reason for hiding this comment

naotoj commented May 14, 2024

openjdk bot commented May 14, 2024

lahodaj left a comment

Choose a reason for hiding this comment

naotoj commented May 15, 2024

openjdk bot commented May 15, 2024

openjdk bot commented May 15, 2024

naotoj commented May 16, 2024

naotoj commented Apr 23, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 23, 2024 •

edited

Loading

mlbridge bot commented Apr 24, 2024 •

edited

Loading