On Wed, 15 Nov 2023 07:41:32 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Overall, this is a very good improvement to the test and looks good to me. I
> just a have a trivial comment about a typo in a code comment, which I've
> added inline.
Thanks for your review, Jaikiran! With these latest, comment-only changes, this
PR is now looking for a sponsor.
> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 78:
>
>> 76: public static final int NAME_LENGTH = 10;
>> 77:
>> 78: // Use a shared LocalDataTime on all entries to save processing time
>
> Hello Eirik, there appears to be a typo in the comment here. It should have
> been `LocalDateTime`.
Fixed
> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 204:
>
>> 202: channel.position(position);
>> 203: // Check for last CEN record
>> 204: if (Arrays.equals(LAST_CEN_COMMENT_BYTES, 0,
>> LAST_CEN_COMMENT_BYTES.length, b, off, len)) {
>
> The way the instance of a `SparseOutputStream` is used in this test, it gets
> passed to the constructor of `ZipOutputStream`. That then means that there is
> no buffering involved when bytes are written out to the output stream
> internally by `ZipOutputStream`. The implementation of `ZipOutputStream`
> writes out the `ZipEntry`'s comment (if any) in one single `write(byte[])`
> call on the outputstream, so it's guaranteed that the `byte[] b` passed in
> here will actually have the entire comment (from `off` to `len`). So this
> `Arrays.equals(...)` check is then guaranteed to pass (when that specific
> entry does get written out). So this check looks good to me.
Thanks, this was a nice observation. I added a short comment to the
SparseOutputStream noting the instances should be passed directly, without any
buffering.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1813123375
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1394673748
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1394674564
