RFR: 8368333: [lworld] Add preview mode to ImageReader and JRT file-system [v3]

David Beaumont duke at openjdk.org
Wed Oct 8 19:58:15 UTC 2025


On Wed, 24 Sep 2025 18:54:02 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Manually deleting ImageReaderFactory (it returned somehow)
>
> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 333:
> 
>> 331:                 // it references have that package marked as preview only.
>> 332:                 // Skipping these entries avoids empty package subdirectories.
>> 333:                 if (previewMode || (flags & FLAGS_IS_PREVIEW_ONLY) == 0) {
> 
> Add utility methods for FLAGS_IS_PREVIEW_ONLY == 0

Done (in next push).

> src/java.base/share/classes/jdk/internal/jimage/ModuleReference.java line 122:
> 
>> 120:     public boolean hasContent() {
>> 121:         return ((flags & FLAGS_HAS_CONTENT) != 0);
>> 122:     }
> 
> isEmpty could avoid introducing "content" as a loosely defined term.
> It would be more similar to directories being empty or lists being empty.

I've bounced between this several times. The thing is that a non-empty package directory will not have content if it only contains other directories, and seeing logic talking about a package being "empty" when it has child directories is weird/confusing. I've actually thought about this naming quite hard and I think it's more misleading to use the term "empty". However, since it's obviously still not clear enough maybe a different name altogether?

How about "hasResources()" or "hasResourceContent()" ?

> src/java.base/share/classes/jdk/internal/jimage/PreviewMode.java line 65:
> 
>> 63:     FOR_RUNTIME() {
>> 64:         @Override
>> 65:         boolean resolve() {
> 
> Each of these overrides creates an extra subclass.
> The alternative is for the resolve method use a switch on the ordinal to return or compute the boolean.

Excellent point, I'll fix this in the next push.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2414886599
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2414884864
PR Review Comment: https://git.openjdk.org/valhalla/pull/1619#discussion_r2414885908


More information about the valhalla-dev mailing list