On Thu, 16 Jan 2025 05:31:44 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert change
>
> test/lib/jdk/test/lib/format/ArrayDiff.java line 110:
>
>> 108: * @return an ArrayDiff instance for the two arrays and formatting
>> parameters provided
>> 109: */
>> 110: @SuppressWarnings({"rawtypes", "unchecked"})
>
> Just wondering where the unchecked warning arises in the code?
It is in the line 123.
> test/lib/jdk/test/lib/hprof/model/JavaHeapObject.java line 42:
>
>> 40: *
>> 41: * @author Bill Foote
>> 42: */
>
> I would suggest deleting any comment blocks that just have the @author tag,
> and deleting the @author elsewhere. All these files (lib/hprof) already have
> an author attribution comment.
done
> test/lib/jdk/test/lib/hprof/parser/ReadBuffer.java line 46:
>
>> 44: public int getInt(long pos) throws IOException;
>> 45: public long getLong(long pos) throws IOException;
>> 46: public void close() throws IOException;
>
> Why was this redefined to throw IOException rather than just Exception?
The javac complains about potential InterruptedException, so I changed type.
See
https://docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html
`Implementers of this interface are also strongly advised to not have the close
method throw
[InterruptedException](https://docs.oracle.com/javase/8/docs/api/java/lang/InterruptedException.html).
This exception interacts with a thread's interrupted status, and runtime
misbehavior is likely to occur if an InterruptedException is
[suppressed](https://docs.oracle.com/javase/8/docs/api/java/lang/Throwable.html#addSuppressed-java.lang.Throwable-).
More generally, if it would cause problems for an exception to be suppressed,
the AutoCloseable.close method should not throw it.`
> test/lib/jdk/test/lib/hprof/parser/Reader.java line 96:
>
>> 94: } else if ((i >>> 8) == GZIP_HEADER_MAGIC) {
>> 95: // Possible gziped file, try decompress it and get the
>> stack trace.
>> 96: String deCompressedFile = "heapdump" +
>> System.currentTimeMillis() + ".hprof";
>
> It is not obvious to me that there may not be a reason for closing all of the
> streams before opening new ones below.
Thanks for catching this. I missed that we try to re-open the same fail in else
branch.
> test/lib/jdk/test/lib/thread/VThreadRunner.java line 100:
>
>> 98: if ((X) ex == ex) {
>> 99: throw (X) ex;
>> 100: }
>
> This doesn't make sense to me.
Sorry for mess, this shouldn't have been committed at all, just some drafting.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23143#discussion_r1918956737
PR Review Comment: https://git.openjdk.org/jdk/pull/23143#discussion_r1918960995
PR Review Comment: https://git.openjdk.org/jdk/pull/23143#discussion_r1918969394
PR Review Comment: https://git.openjdk.org/jdk/pull/23143#discussion_r1918976104
PR Review Comment: https://git.openjdk.org/jdk/pull/23143#discussion_r1918981965
