On Mon, 30 Mar 2026 08:39:43 GMT, David Beaumont <[email protected]> wrote:

>> src/java.base/share/classes/jdk/internal/jimage/ImageLocation.java line 132:
>> 
>>> 130:      *     is present.
>>> 131:      * @return flags for the ATTRIBUTE_PREVIEW_FLAGS attribute.
>>> 132:      */
>> 
>> getPreviewFlags looks very strange. Is this motivated by tests? (I wouldn't 
>> expect anything other than the jlink image writer to be concerned with this).
>
> It's where it is because it's using the encapsulated FLAG_XXX constants. It's 
> split out as this separate util function because the existing code has 2, 
> completely separate locations, at which it needs the flags calculated. The 
> docs for the method explain it:
> 
> 
>      * <p>Since preview flags are calculated separately for resource nodes and
>      * directory nodes (in two quite different places) it's useful to have a
>      * common helper.
> 
> 
> If the existing code were re-worked to avoid processing resource and 
> directory nodes in completely difference classes (`ImageResourcesTree` and 
> `ImageFileCreator`), this could probably be neatened up. Any other solution 
> would involve code duplication and/or breaking encapsulation.

I would prefer if we could replace this with something simpler that doesn't 
have a user provider predicate. Can you work through the usage in the jlink 
image writer to see we could make t his simpler and easier to 
understand/maintain.

>> src/java.base/share/classes/jdk/internal/jimage/ImageReader.java line 275:
>> 
>>> 273:         // preview-only nodes. This is used to add preview-only 
>>> content to
>>> 274:         // directories as they are completed.
>>> 275:         private final Map<String, Directory> previewDirectoriesToMerge;
>> 
>> I think it would be better if we could use the stick with the term "overlay" 
>> when speaking of the resources in /META-INF/preview as it is confusing to 
>> see "merge" in some of the method names and comments.
>> 
>> Also would it be possible to explain what you mean by "relativized mapping" 
>> here?
>
> This is a "merging" operation though. And overlay would be a view of two bits 
> of data to make them look the same, but this is data which is merged with 
> other data to form a single result that's not a view of several things.
> 
> I'm using "relativized" in the same sense as `Path.relativize()` but I'll 
> explain what it's relative to.

The terminology is very confusing. The META-INF/preview tree is an overlay, 
just like a versioned section in a MR JAR. So hopefully we can go through all 
naming and comments in this PR to get consistent use of the terms.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r3008422152
PR Review Comment: https://git.openjdk.org/jdk/pull/29414#discussion_r3008430727

Reply via email to