On Wed, 7 Feb 2024 01:52:06 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this doc-only change which updates the javadoc
>> of several classes in `java.util.jar` and `java.util.zip` to specify their
>> behaviour when `null` arguments are passed to the constructor or methods of
>> those classes?
>>
>> For these updated classes, I have individually checked that they indeed
>> throw a `NullPointerException` when `null` is passed to their constructor or
>> methods. The couple of places where `null` is accepted have been updated to
>> mention that `null` is allowed.
>
> 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
This mostly looks okay, just a few comments.
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?
src/java.base/share/classes/java/util/zip/DeflaterInputStream.java line 80:
> 78: // "in" being null isn't allowed. we use a null check for "in"
> 79: // merely to avoid an unnecessary instance creation of the
> Deflater
> 80: // for such erroneous cases.
I see this same comment has been added in 3 places but it's not easy to read
and I don't think is needed.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17728#pullrequestreview-1867349428
PR Review Comment: https://git.openjdk.org/jdk/pull/17728#discussion_r1481197701
PR Review Comment: https://git.openjdk.org/jdk/pull/17728#discussion_r1481189478
