On Tue, 20 May 2025 21:57:45 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> `java.io.Console` uses the charset specified by the `stdout.encoding` system
>> property for both input and output. While this is generally sufficient,
>> since Console is intended for interactive terminal use, some platforms allow
>> different encodings to be configured for input and output. In such cases,
>> using a single encoding may lead to incorrect behavior when reading from the
>> terminal. To address this, the newly introduced system property,
>> `stdin.encoding`, should be used specifically for input where appropriate.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflects review comments
src/java.base/share/classes/java/io/Console.java line 67:
> 65: * stdout.encoding}, in which case read operations use the {@code Charset}
> 66: * designated by {@code stdin.encoding}.
> 67: * <p>
`Console.charset()` states "The returned charset is used for interpreting the
input and output source (e.g., keyboard and/or display) specified by the host
environment or user, which defaults to the one based on stdout.encoding." If
_stdin.encoding_ is set otherwise, this is no longer true, so I think this
method may need a wording update as well.
test/jdk/java/io/Console/StdinEncodingTest.java line 38:
> 36: * @test
> 37: * @bug 8356985
> 38: * @summary Tests if "stdin.encoding" is reflected for reading
Would be helpful to include why the test is limited to only linux and mac,
> "expect" command in Windows/Cygwin does not work as expected. Ignoring tests
> on Windows.
test/jdk/java/io/Console/StdinEncodingTest.java line 51:
> 49: // check "expect" command availability
> 50: var expect = Paths.get("/usr/bin/expect");
> 51: if (!Files.exists(expect) || !Files.isExecutable(expect)) {
Could use `Assumptions.assumeTrue` here. Condition becomes more readable as:
`Files.exists(expect) && Files.isExecutable(expect)`
test/jdk/java/io/Console/StdinEncodingTest.java line 56:
> 54:
> 55: // invoking "expect" command
> 56: var testSrc = System.getProperty("test.src", ".");
The test already imports the JDK test lib, can we just replace this and the
below ocurrences with `jdk.test.lib.Utils.TEST_SRC/JDK/CLASSES` directly?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2099438513
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2099441523
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2099447047
PR Review Comment: https://git.openjdk.org/jdk/pull/25271#discussion_r2099443105
