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

Reply via email to