On Tue, 1 Jul 2025 14:29:28 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Updates based on feedback.
>
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 62:
>
>> 60: import jdk.internal.module.ModuleHashes.HashSupplier;
>> 61:
>> 62: import static java.util.Objects.requireNonNull;
>
> The existing Objects.requireNonNull are okay, no need to change them all as
> it is nothing to do with the changes.
Done.
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 251:
>
>> 249: }
>> 250:
>> 251: private static Stream<ModuleInfo.Attributes> getModuleAttributes() {
>
> Can you rename this to allModuleAttributes and add a method description to
> match the other methods.
Done.
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 257:
>
>> 255: .getChildNames()
>> 256: .map(mn -> getNode(reader, mn + "/module-info.class"))
>> 257: // This fails with ISE if the node isn't a resource
>> (corrupt JImage).
>
> Can you drop this comment and check getNode to thrown an Error (ISE isn't
> right when we have a broken image).
`getNode()` is called twice and while it can throw the error, there's no nice
way for `getResourceBuffer()` to be that strict (it's a separate API).
After fiddling a bit I reworked the module attribute loading into a separate
helper and caught & escalated any exceptions relating to that into `Error`.
The only question now is that `ModuleInfo.read()` has at least one other
runtime exception (`InvalidModuleDescriptorException`) it can throw which we
could catch here. Should I add that to the list?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2178463701
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2178463291
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2178462973
