On Wed, 7 Feb 2024 09:44:25 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> make <p> usage consistent with other similar usages in the file
>
> src/java.base/share/classes/java/util/zip/CheckedOutputStream.java line 48:
>
>> 46: * value to either {@code out} or {@code cksum} will cause
>> 47: * a {@link NullPointerException} to be thrown from the
>> 48: * {@code write} methods of this {@code CheckedOutputStream}.
>
> What is the reason for specifying the NPE in the method description rather
> than a throws?
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() throws IOException {
* @param len the maximum number of bytes read
* @return the actual number of bytes read, or -1 if the end
* of the stream is reached.
- * @throws NullPointerException If {@code buf} is {@code null}.
+ * @throws NullPointerException If {@code buf} is {@code null} or
+ * if this {@code CheckedInputStream} was
+ * constructed with a {@code null} value for {@code in} or
{@code cksum}.
* @throws IndexOutOfBoundsException If {@code off} is negative,
* {@code len} is negative, or {@code len} is greater than
* {@code buf.length - off}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17728#discussion_r1481237216
