On Wed, 23 Jul 2025 06:37:34 GMT, Alan Bateman <al...@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