On Fri, 22 Nov 2024 17:25:54 GMT, Calvin Cheung <[email protected]> wrote:
>> Currently, when retrieving a ClassFileStream during runtime, we call into
>> the zip library to retrieve the stream based only on a class name. This
>> doesn't work well if the class is in a multi-release jar under a versioned
>> directory such as `META-INF/versions/9/Foo.class`. To address this issue,
>> this change calls the java api `ClassLoader.getResourceAsStream()` to
>> retrieve the stream.
>>
>> Passed tiers 1 - 4 testing.
>
> 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.
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());
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1855752520
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1855752795
PR Review Comment: https://git.openjdk.org/jdk/pull/22262#discussion_r1855756738
