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