On Thu, 21 Aug 2025 19:05:44 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> This PR addresses several issues found while adding unit tests for
>> ExplodedImage.
>> I have added review comments for changes (some of which are a little
>> preferential, but bring the code into line with things like ImageReader in
>> terms of the name choices for variables).
>> Later it is likely that this code will be adapted for the up-coming preview
>> mode support in Valhalla, so having it unit-testable is important.
>
> src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 193:
>
>> 191: * path references a file which must be hidden in the node
>> hierarchy.
>> 192: */
>> 193: Node createModulesNode(String name, Path path) {
>
> This and other methods could be `private` if not used outside the class.
Done.
There's little consistency in a lot of the access modifiers in the code around
here, so I'm defaulting to "don't touch" since I don't know what is, or is not
the accepted style. There are lots of other methods here which could/should be
private, and it's probably better to raise a PR to go over the entire package
and do a sweep for consistency rather than picking these off one-at-a-time.
> src/java.base/share/classes/jdk/internal/jrtfs/ExplodedImage.java line 195:
>
>> 193: Node createModulesNode(String name, Path path) {
>> 194: assert !nodes.containsKey(name) : "Node must not already exist:
>> " + name;
>> 195: assert name.startsWith(MODULES) && name.length() >
>> MODULES.length() : "Invalid modules name: " + name;
>
> startsWith(MODUELES) && name.length(...) might be worth a utility method.
> Only 2 uses though.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2309546717
PR Review Comment: https://git.openjdk.org/jdk/pull/26757#discussion_r2309547063
