On Wed, 7 Feb 2024 10:47:31 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> These 2 classes, the `CheckedInputStream` and the `CheckedOutputStream` are
>> slightly different from the rest of the classes in this changeset. This
>> javadoc here is for the constructor of the `CheckedInputStream`. The
>> implementation of the constructor just blindly assigns the passed argument
>> to an internal member field and doesn't do any null check on the passed
>> arguments (any subsequent usage of these instance fields within that class,
>> then leads to a NullPointerException). So the constructor isn't throwing any
>> `NullPointerException` here and thus adding a `@throws` here would be
>> incorrect. In theory, we could just change the implementation of this
>> constructor to throw a `NullPointerException` if either of these arguments
>> were null, but that might mean a potential breakage of some weird usage of
>> the CheckedInputStream. So I decided to add the `NullPointerException`
>> detail to the constructor description.
>>
>> Do you think we should instead do something like this for this class and the
>> `CheckedOutputStream` class:
>>
>>
>> diff --git
>> a/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
>> b/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
>> index 10f72b416d1..76cba26986e 100644
>> --- a/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
>> +++ b/src/java.base/share/classes/java/util/zip/CheckedInputStream.java
>> @@ -41,10 +41,7 @@ public class CheckedInputStream extends FilterInputStream
>> {
>> private final Checksum cksum;
>>
>> /**
>> - * Creates an input stream using the specified Checksum. A null
>> - * value to either {@code in} or {@code cksum} will cause
>> - * a {@link NullPointerException} to be thrown from the
>> - * {@code read} methods of this {@code CheckedInputStream}.
>> + * Creates an input stream using the specified Checksum.
>> * @param in the input stream
>> * @param cksum the Checksum
>> */
>> @@ -57,6 +54,8 @@ public CheckedInputStream(InputStream in, Checksum cksum) {
>> * Reads a byte. Will block if no input is available.
>> * @return the byte read, or -1 if the end of the stream is reached.
>> * @throws IOException if an I/O error has occurred
>> + * @throws NullPointerException if this {@code CheckedInputStream}
>> was
>> + * constructed with a {@code null} value for {@code in} or
>> {@code cksum}
>> */
>> public int read() throws IOException {
>> int b = in.read();
>> @@ -75,7 +74,9 @@ public int read() thro...
>
>> These 2 classes, the `CheckedInputStream` and the `CheckedOutputStream` are
>> slightly different from the rest of the classes in this changeset. This
>> javadoc here is for the constructor of the `CheckedInputStream`. The
>> implementation of the constructor just blindly assigns the passed argument
>> to an internal member field and doesn't do any null check on the passed
>> arguments (any subsequent usage of these instance fields within that class,
>> then leads to a NullPointerException).
>
> The superclasses FilterInputStream and FilterOutputStream have a a protected
> field for the underlying stream so it's possible for a subclass to set the
> underlying stream lazily. I can't recall seeing code in the wild availing of
> that but it is possible.
>
> CheckedInputStream and CheckedOutputStream date from JDK 1.1 and it's not
> clear what the intention was. My guess is that it was an oversight/bug to not
> check for null when constructing directly. Fixing this will help catch buggy
> code but it does mean a behavior change. I think we have to keep existing
> behavior for the subclassing case because it is possible for the subclass to
> set the stream lazily.
Given that subclasses could set these fields lazily (however remote the case
might be), do you think we should then not specify the `NullPointerException`
for the read methods on these 2 classes. In which case I can exclude these 2
classes from this PR.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17728#discussion_r1481314236
