Conversation
|
👋 Welcome back dbeaumont! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
added
hotspot-runtime
|
@david-beaumont The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
| if (name.startsWith(PACKAGES_PREFIX + "/")) { | ||
| throw new IllegalArgumentException( | ||
| "Package sub-directory flags handled separately: " + name); | ||
| // '/packages/xxx' entries have flags, but not calculated here. |
There was a problem hiding this comment.
Not an error now, return zero to leave packages flags alone though.
| return attributes; | ||
| } | ||
|
|
||
| public static byte[] compress(long[] attributes) { |
There was a problem hiding this comment.
Moved from parent class (it's only used here).
| } | ||
| // Jimage cannot encode resources where the module name is "modules" or "packages". | ||
| // Module names should only come from Module instances or other trusted sources. | ||
| #ifdef ASSERT |
There was a problem hiding this comment.
Frustrating need for ifdef to avoid compilation failure due to unused consts.
| } | ||
|
|
||
| public void addLocation( | ||
| public ImageLocationWriter addLocation( |
There was a problem hiding this comment.
Returned so we can add the package flags early (special case).
| } | ||
| } | ||
|
|
||
| private void generateLocationFlags() { |
There was a problem hiding this comment.
This replaces the 2 calls (I can also tidy the API to pass the keySet instead of a predicate if desired).
| res.path(), p -> resultResources.findEntry(p).isPresent()); | ||
| duplicates.add(path); | ||
| writer.addLocation(path, offset[0], compressedSize, uncompressedSize, locFlags); | ||
| writer.addLocation(path, offset[0], compressedSize, uncompressedSize); |
There was a problem hiding this comment.
This gets slightly simpler in this version.
| .addAttribute(ATTRIBUTE_OFFSET, contentOffset) | ||
| .addAttribute(ATTRIBUTE_COMPRESSED, compressedSize) | ||
| .addAttribute(ATTRIBUTE_UNCOMPRESSED, uncompressedSize) | ||
| .addAttribute(ATTRIBUTE_PREVIEW_FLAGS, previewFlags); |
There was a problem hiding this comment.
No longer set flags during init. I think this is a weakness of the change because there's no longer a forcing function and now code code accidentally fail to set values. This is, however, quite well tested.
| static final class Tree { | ||
| private static final String PREVIEW_PREFIX = "META-INF/preview/"; | ||
|
|
||
| private final Map<String, Node> directAccess = new HashMap<>(); |
There was a problem hiding this comment.
This name has always been confusing, I finally realized that "direct" means "directory", not "direct".
| // Flags are more complex to calculate than other cases, so do them | ||
| // early (the other flags are calculated in the writer itself). | ||
| ImageLocationWriter location = writer.addLocation(current.getPath(), offset, 0, size); | ||
| location.addAttribute(ATTRIBUTE_PREVIEW_FLAGS, ImageLocation.getPackageFlags(links)); |
There was a problem hiding this comment.
This is the "sore thumb" of this change. We need to return the writer from addLocation() so we can poke the package flags in here, because it's hard to generate them after this point.
| // Normal directory entries have 4-byte entries (offset only). | ||
| int size = ret.length * 4; | ||
| writer.addLocation(current.getPath(), offset, 0, size, locFlags); | ||
| writer.addLocation(current.getPath(), offset, 0, size); |
There was a problem hiding this comment.
Slightly simpler in this change.
| } | ||
|
|
||
| private ImageLocationWriter addAttribute(int kind, long value) { | ||
| private static byte[] compress(long[] attributes) { |
There was a problem hiding this comment.
The moved method from the parent class (which never needs it).
1 participant
Alternate preview flags prototype.
Progress
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30718/head:pull/30718$ git checkout pull/30718Update a local copy of the PR:
$ git checkout pull/30718$ git pull https://git.openjdk.org/jdk.git pull/30718/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30718View PR using the GUI difftool:
$ git pr show -t 30718Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30718.diff