RFR: 8360037: Refactor ImageReader in preparation for Valhalla support [v4]

[David Beaumont]

On Wed, 2 Jul 2025 12:51:27 GMT, Alan Bateman <alanb at 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