On Mon, 24 May 2021 00:33:06 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Methods are added to java.lang.Process to read and write characters and
>> lines from and to a spawned Process.
>> The Charset used to encode and decode characters to bytes can be specified
>> or use the
>> operating system native encoding as is available from the "native.encoding"
>> system property.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added throws for NPE, delegated zero-arg methods that use native.encoding to
> the 1-arg method with a charset, added tests for Redirect cases where the
> streams are null-input or null-output streams.
src/java.base/share/classes/java/lang/Process.java line 178:
> 176: *
> 177: * <p>This method delegates to {@link #inputReader(Charset)} using
> the
> 178: * {@link Charset} named by the {@systemProperty native.encoding}
I thought that `{@systemProperty ... }` was supposed to be used at the place
where the system property is defined, and *not* at the place where it's being
used? I might be mistaken, but if I'm not - then there would be several places
to fix in this file.
src/java.base/share/classes/java/lang/Process.java line 226:
> 224: *
> 225: * @param charset the {@code Charset} used to decode bytes to
> characters, not null
> 226: * @return a BufferedReader for the standard output of the process
> using the {@code charset}
`{@code BufferedReader}`
src/java.base/share/classes/java/lang/Process.java line 231:
> 229: */
> 230: public BufferedReader inputReader(Charset charset) {
> 231: return new BufferedReader(new
> InputStreamReader(getInputStream(), charset));
I'd suggest calling Objects.requireNonNull(charset) first thing in this method
instead of relying on the InputStreamReader constructor to throw the NPE.
src/java.base/share/classes/java/lang/Process.java line 283:
> 281: *
> 282: * @param charset the {@code Charset} used to decode bytes to
> characters, not null
> 283: * @return a BufferedReader for the standard error of the process
> using the {@code charset}
`{@code BufferedReader}`
src/java.base/share/classes/java/lang/Process.java line 288:
> 286: */
> 287: public BufferedReader errorReader(Charset charset) {
> 288: return new BufferedReader(new
> InputStreamReader(getErrorStream(), charset));
Same remark here: I'd suggest calling Objects.requireNonNull(charset) first
thing in this method instead of relying on the InputStreamReader constructor to
throw the NPE.
src/java.base/share/classes/java/lang/Process.java line 317:
> 315: * {@linkplain BufferedWriter#flush flush} is called.
> 316: *
> 317: * @return a BufferedWriter to the standard input of the process
> using the charset
`{@code BufferedWriter}`
src/java.base/share/classes/java/lang/Process.java line 351:
> 349: *
> 350: * @param charset the {@code Charset} to encode characters to bytes,
> not null
> 351: * @return a BufferedWriter to the standard input of the process
> using the {@code charset}
`{@code BufferedWriter}`
src/java.base/share/classes/java/lang/Process.java line 356:
> 354: */
> 355: public BufferedWriter outputWriter(Charset charset) {
> 356: return new BufferedWriter(new
> OutputStreamWriter(getOutputStream(), charset));
I'd suggest calling Objects.requireNonNull(charset) first thing in this method
instead of relying on the InputStreamWriter constructor to throw the NPE.
src/java.base/share/classes/java/lang/Process.java line 459:
> 457: * <p>
> 458: * Invoking this method on {@code Process} objects returned by
> 459: * {@link ProcessBuilder#start()} and {@link Runtime#exec} forcibly
> terminate
Should the link to `Runtime#exec` be fixed in the same manner, here and
elsewhere in this file?
-------------
PR: https://git.openjdk.java.net/jdk/pull/4134
