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

[David Beaumont]

On Wed, 23 Jul 2025 06:37:34 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Feedback changes.
>
> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 259:
> 
>> 257:         ImageReader reader = SystemImage.reader();
>> 258:         try {
>> 259:             return reader.findNode("/modules").getChildNames().map(mn -> readModuleAttributes(reader, mn));
> 
> Can you put the line over 3 lines so that it's easier to see the pipeline.

Done.

> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 265:
> 
>> 263:     }
>> 264: 
>> 265:     // Every module is required to have a valid module-info.class.
> 
> The methods in this class use use /** ..*/ in the method descriptions so would prefer if the changes change that. Here the method should really say that it returns the module's module-info, returning a holder for its class file attributes.

That comment was JavaDoc, just an implementation note (since it's private and no separate doc would be created for it). I fleshed it out a bit though.

> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 276:
> 
>> 274:             err = e;
>> 275:         }
>> 276:         throw new Error("Missing or invalid module-info.class for module: " + moduleName, err);
> 
> I assume this can move into the catch block.

No because the return is conditional above "node != null && node.isResource()".

> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 457:
> 
>> 455:          * a non-resource node, then {@code null} is returned.
>> 456:          */
>> 457:         private ImageReader.Node findResourceNode(ImageReader reader, String name) throws IOException {
> 
> I think the usage would be clear if this were renamed to findResource.

Done.

> src/java.base/share/classes/jdk/internal/module/SystemModuleFinders.java line 470:
> 
>> 468:         public Optional<ByteBuffer> read(String name) throws IOException {
>> 469:             ImageReader reader = SystemImage.reader();
>> 470:             return Optional.ofNullable(findResourceNode(reader, name)).map(reader::getResourceBuffer);
> 
> Style wise, can you put the .map(...) on the next line so this is easier to read.

Done. Is there any documented best practice for this sort of line splitting (since there isn't an enforce formatter).

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231629846
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231634425
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231631736
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231628586
PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2231626965