On Mon, 25 Nov 2024 04:11:20 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Calvin Cheung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update comments
>
> src/hotspot/share/cds/filemap.cpp line 2718:
>
>> 2716: // The result should be a [B
>> 2717: assert(obj->is_typeArray(), "just checking");
>> 2718: assert(TypeArrayKlass::cast(obj->klass())->element_type() == T_BYTE,
>> "just checking");
>
> This seems unnecessary. What kind of errors are you trying to detect with
> this? The method signature indicates it returns a byte-array.
I adapted the asserts from diagnosticCommand.cpp. I'm keeping the first assert.
Is it ok?
> src/hotspot/share/cds/filemap.cpp line 2728:
>
>> 2726: return new ClassFileStream(buffer,
>> 2727: len,
>> 2728: cpe->name());
>
> Suggestion:
>
> return new ClassFileStream(buffer, len, cpe->name());
Fixed.
> src/java.base/share/classes/java/lang/ClassLoader.java line 1698:
>
>> 1696: } catch (IOException e) {
>> 1697: return null;
>> 1698: }
>
> Any `IOException` should just be left pending so that it gets reported
> eventually. Otherwise you are returning null here but asserting in the VM
> that null is never returned.
I've changed the java method to throw IOException and removed the try-catch
statements.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1857233789
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1857233832
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1857233887
