On Wed, 2 Jul 2025 12:51:27 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Small feedback related tweaks.
>
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 266:
>
>> 264:
>> 265: // The nodes we are processing must exist (every module must have a
>> module-info.class).
>> 266: private static ModuleInfo.Attributes
>> loadModuleAttributes(ImageReader reader, String moduleName) {
>
> Would you mind renaming this to readModuleAttributes as it's confusing to
> switch to using "load" here.
>
> Also can you fix the comment as it is very confusing to to say "The nodes we
> are processing must exist" when the method is simple called to read and parse
> a module-info.
Done.
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line
> 458:
>
>> 456: * returned even if a non-resource node exists with the given
>> name.
>> 457: */
>> 458: private Optional<ImageReader.Node> findResourceNode(ImageReader
>> reader, String name) throws IOException {
>
> If you change this method use SystemImage.reader (like containsResource does)
> then it would be a bit nicer. It's okay to change this method to return null
> when not found too, the read method can box.
I changed the return to plain value, but not the use of ImageReader. The caller
needs the reader instance too, so it can't be moved into the
`findResourceNode()`, only duplicated there.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2180118262
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2180122848
