On Wed, 17 Jun 2026 11:06:52 GMT, Alan Bateman <[email protected]> wrote:
>> Chen Liang has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - Review remarks >> - Merge branch 'lworld' of https://github.com/openjdk/valhalla into >> cleanup/lw-jrtfs >> - Cleanup ExplodedImage and enhance SystemImageTest > > src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 333: > >> 331: >> 332: // Ensures this is a name taking form /modules/... with no trailing >> slash. >> 333: private static boolean isRealPathName(String name) { > > "real path" has connotations, suggesting it might be testing if the path is a > canonicalized path. Something simple isModulesPath is okay or leave it as it > was. I think `PathInModules` is the best way to describe these paths. > src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 398: > >> 396: String moduleName = moduleDir.getFileName().toString(); >> 397: // Make sure "/modules/<moduleName>" is created >> 398: Objects.requireNonNull(findNode(MODULES + moduleName)); > > This will throw NPE if the module is not found in the /modules tree. > Pre-existing issue, that I don't think is possible right now, but would be > better to throw IAE here. Hmm, I don't know why this should throw IAE - this is used at initialization of Jrtfs, throwing an IAE through and wrapping in an InternalError is not helpful either. I think we can safely eliminate this call as the findNode call in packages loop should accomplish the same effect. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2545#discussion_r3429365413 PR Review Comment: https://git.openjdk.org/valhalla/pull/2545#discussion_r3429362487
