Re: RFR: 8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base

[Alan Bateman]

On Sat, 17 May 2025 19:42:39 GMT, Nizar Benalla  wrote:

> Please review this patch to fix some `javadoc` bugs in `java.base`.
> Certain `@link` tags used to refer to private fields instead of public APIs.
> 
> A couple of `@see` tags in the [serialization 
> page](https://download.java.net/java/early_access/jdk25/docs/api/serialized-form.html#java.lang.invoke.MethodType)
>  referred to private methods, I updated the javadoc in a way to not change 
> the way it is displayed to users but also remove `@link` tags to non-included 
> types.
> 
> TIA

src/java.base/share/classes/java/util/concurrent/ForkJoinTask.java line 138:

> 136:  * {@link #isCompletedAbnormally} is true if a task was either
> 137:  * cancelled or encountered an exception, in which case {@link
> 138:  * #getException()} will return either the encountered exception or

Looks right too, the private overload of getException was added in JDK 22.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25287#discussion_r2097113958

Re: RFR: 8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base

[Alan Bateman]

On Sat, 17 May 2025 19:42:39 GMT, Nizar Benalla  wrote:

> Please review this patch to fix some `javadoc` bugs in `java.base`.
> Certain `@link` tags used to refer to private fields instead of public APIs.
> 
> A couple of `@see` tags in the [serialization 
> page](https://download.java.net/java/early_access/jdk25/docs/api/serialized-form.html#java.lang.invoke.MethodType)
>  referred to private methods, I updated the javadoc in a way to not change 
> the way it is displayed to users but also remove `@link` tags to non-included 
> types.
> 
> TIA

src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 2023:

> 2021: /**
> 2022:  * Provides a service with one or more implementations. The 
> package for
> 2023:  * each {@link Provides#providers() provider} (or provider 
> factory) is

okay

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25287#discussion_r2097120353

Re: RFR: 8355563: VectorAPI: Refactor current implementation of subword gather load API

[Emanuel Peter]

On Tue, 20 May 2025 02:22:13 GMT, Xiaohong Gong  wrote:

>> Ping again~  could any one please take a look at this PR？ Thanks a lot!
>
>> Hi @XiaohongGong , Very nice work!, Looks good to me, will do some testing 
>> and get back.
>> 
>> Do you have any idea about following regression?
>> 
>> ```
>> GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
>> 6455844.814   48311.847  0.86
>> GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
>> 256   15139.459   13009.848  0.85
>> GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
>> 1024   3861.8343284.944  0.85
>> GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
>> 4096938.665 817.673  0.87
>> ```
>> 
>> Best Regards
> 
> Yes, I also observed such regression. After analyzing, I found it was caused 
> by the java side changes, which influences the range check elimination inside 
> `IntVector.fromArray()` and `ByteVector.intoArray()` in the benchmark. The 
> root cause is the counted loop in following benchmark case is not recognized 
> by compiler as expected:
> 
> public void microByteGather256() {
> for (int i = 0; i < SIZE; i += B256.length()) {
> ByteVector.fromArray(B256, barr, 0, index, i)
> .intoArray(bres, i);
> }
>  }
> ``` 
> The loop iv phi node is not recognized successfully when C2 recognize the 
> counted loop pattern, because it was casted twice with `CastII` in this case. 
> The ideal graph looks like:
> 
>Loop
>\
> \/ -
>\  / |
>Phi  |
>  |  | 
>   CastII|
>   | |
>   CastII|
>   | |
>  \   ConI   |
> \  ||
> AddVI   |
>   |-| 
>   
> 
> Relative code is 
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/loopnode.cpp#L1667.
>  
> 
> Befor...

@XiaohongGong Thanks for splitting this one out, and for investigating the 
regressions here.

Putting the permalink here, fixed to the current change (the link you pasted 
will always refer to the newest, which may later on point to the wrong line 
when lines above are inserted / deleted):
https://github.com/openjdk/jdk/blob/7077535c0b0a6ea0a2a167f9135b1504a3d71fb3/src/hotspot/share/opto/loopnode.cpp#L1659-L1661

I wonder if we should just use `Node::uncast` there? But I'm quite unsure about 
that.

> Yes, I also observed such regression.
It would be nice if you proactively mentioned regressions, so it does not have 
to be pointed out by reviewers.

For me, it could be ok to fix it in a follow-up patch. I think we are too close 
to RDP1 for JDK25 now anyway, and so we could push this patch here into JDK26, 
and then we have enough time in JDK26 to investigate the regression. Even 
better would be if we could do the other patch first, so we never even 
encounter a regression.

-

PR Comment: https://git.openjdk.org/jdk/pull/25138#issuecomment-2893017948

Re: RFR: 8355563: VectorAPI: Refactor current implementation of subword gather load API

[Xiaohong Gong]

On Tue, 20 May 2025 02:22:13 GMT, Xiaohong Gong  wrote:

>> Ping again~  could any one please take a look at this PR？ Thanks a lot!
>
>> Hi @XiaohongGong , Very nice work!, Looks good to me, will do some testing 
>> and get back.
>> 
>> Do you have any idea about following regression?
>> 
>> ```
>> GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
>> 6455844.814   48311.847  0.86
>> GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
>> 256   15139.459   13009.848  0.85
>> GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
>> 1024   3861.8343284.944  0.85
>> GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
>> 4096938.665 817.673  0.87
>> ```
>> 
>> Best Regards
> 
> Yes, I also observed such regression. After analyzing, I found it was caused 
> by the java side changes, which influences the range check elimination inside 
> `IntVector.fromArray()` and `ByteVector.intoArray()` in the benchmark. The 
> root cause is the counted loop in following benchmark case is not recognized 
> by compiler as expected:
> 
> public void microByteGather256() {
> for (int i = 0; i < SIZE; i += B256.length()) {
> ByteVector.fromArray(B256, barr, 0, index, i)
> .intoArray(bres, i);
> }
>  }
> ``` 
> The loop iv phi node is not recognized successfully when C2 recognize the 
> counted loop pattern, because it was casted twice with `CastII` in this case. 
> The ideal graph looks like:
> 
>Loop
>\
> \/ -
>\  / |
>Phi  |
>  |  | 
>   CastII|
>   | |
>   CastII|
>   | |
>  \   ConI   |
> \  ||
> AddVI   |
>   |-| 
>   
> 
> Relative code is 
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/loopnode.cpp#L1667.
>  
> 
> Befor...

> @XiaohongGong Thanks for splitting this one out, and for investigating the 
> regressions here.
> 
> Putting the permalink here, fixed to the current change (the link you pasted 
> will always refer to the newest, which may later on point to the wrong line 
> when lines above are inserted / deleted):
> 
> https://github.com/openjdk/jdk/blob/7077535c0b0a6ea0a2a167f9135b1504a3d71fb3/src/hotspot/share/opto/loopnode.cpp#L1659-L1661
> 
> I wonder if we should just use `Node::uncast` there? But I'm quite unsure 
> about that.

Sounds good to me. I will have a deep investigation for it. Thanks!



> > Yes, I also observed such regression.
> > It would be nice if you proactively mentioned regressions, so it does not 
> > have to be pointed out by reviewers.
> 
> For me, it could be ok to fix it in a follow-up patch. I think we are too 
> close to RDP1 for JDK25 now anyway, and so we could push this patch here into 
> JDK26, and then we have enough time in JDK26 to investigate the regression. 
> Even better would be if we could do the other patch first, so we never even 
> encounter a regression.

Sounds good to me. Thanks!

-

PR Comment: https://git.openjdk.org/jdk/pull/25138#issuecomment-2893026228

Re: RFR: 8355223: Improve documentation on @IntrinsicCandidate [v6]

[Jaikiran Pai]

On Wed, 30 Apr 2025 22:26:30 GMT, Chen Liang  wrote:

>> In offline discussion, we noted that the documentation on this annotation 
>> does not recommend minimizing the intrinsified section and moving whatever 
>> can be done in Java to Java; thus I prepared this documentation update, to 
>> shrink a "TLDR" essay to something concise for readers, such as pointing to 
>> that list at `vmIntrinsics.hpp` instead of "a list".
>
> 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 eight additional commits since 
> the last revision:
> 
>  - Move intrinsic to be a subsection; just one most common function of the 
> annotation
>  - Merge branch 'master' of https://github.com/openjdk/jdk into 
> doc/intrinsic-candidate
>  - Merge branch 'master' of https://github.com/openjdk/jdk into 
> doc/intrinsic-candidate
>  - Update 
> src/java.base/share/classes/jdk/internal/vm/annotation/IntrinsicCandidate.java
>
>Co-authored-by: Raffaello Giulietti 
>  - Shorter first sentence
>  - Updates, thanks to John
>  - Refine validation and defensive copying
>  - 8355223: Improve documentation on @IntrinsicCandidate

src/java.base/share/classes/jdk/internal/vm/annotation/IntrinsicCandidate.java 
line 50:

> 48:  * For example, the bytecodes of a candidate method may be executed by 
> lower
> 49:  * compilation tiers of VM execution, while higher compilation tiers may 
> replace
> 50:  * the bytecodes with specialized assembly code and/or compiler IR.  
> Therefore,

> while higher compilation tiers may replace the bytecodes with specialized 
> assembly code and/or compiler IR

Is there ever a case, where for a `@IntrinsicCandidate` method, the runtime 
will choose to execute the instrinsic for that method for a certain duration 
and then at a later point in time replace the intrinsic with compiler generated 
code? In other words, once the runtime executes the intrinsic implementation 
for a `@IntrinsicCandidate` method, will the method's implementation be 
switched to anything else during the lifetime of an application?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24777#discussion_r2097016882

Re: RFR: 8355223: Improve documentation on @IntrinsicCandidate [v6]

[Jaikiran Pai]

On Fri, 16 May 2025 19:55:58 GMT, John R Rose  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 eight additional 
>> commits since the last revision:
>> 
>>  - Move intrinsic to be a subsection; just one most common function of the 
>> annotation
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into 
>> doc/intrinsic-candidate
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into 
>> doc/intrinsic-candidate
>>  - Update 
>> src/java.base/share/classes/jdk/internal/vm/annotation/IntrinsicCandidate.java
>>
>>Co-authored-by: Raffaello Giulietti 
>>  - Shorter first sentence
>>  - Updates, thanks to John
>>  - Refine validation and defensive copying
>>  - 8355223: Improve documentation on @IntrinsicCandidate
>
> src/java.base/share/classes/jdk/internal/vm/annotation/IntrinsicCandidate.java
>  line 47:
> 
>> 45:  * intrinsics necessary.
>> 46:  * 
>> 47:  * Intrinsification may never happen, or happen at any moment during 
>> execution.
> 
> s/or happen/or may happen/ (easier to parse)

Hello John, are there are any hotspot VM flags that can be enabled to check 
whether or not intrinsification happen for a particular method during the 
lifetime of an application? Should any of those flags be documented in this 
proposed text?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24777#discussion_r2097001318

Re: RFR: 8355223: Improve documentation on @IntrinsicCandidate [v6]

[Jaikiran Pai]

On Wed, 30 Apr 2025 22:26:30 GMT, Chen Liang  wrote:

>> In offline discussion, we noted that the documentation on this annotation 
>> does not recommend minimizing the intrinsified section and moving whatever 
>> can be done in Java to Java; thus I prepared this documentation update, to 
>> shrink a "TLDR" essay to something concise for readers, such as pointing to 
>> that list at `vmIntrinsics.hpp` instead of "a list".
>
> 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 eight additional commits since 
> the last revision:
> 
>  - Move intrinsic to be a subsection; just one most common function of the 
> annotation
>  - Merge branch 'master' of https://github.com/openjdk/jdk into 
> doc/intrinsic-candidate
>  - Merge branch 'master' of https://github.com/openjdk/jdk into 
> doc/intrinsic-candidate
>  - Update 
> src/java.base/share/classes/jdk/internal/vm/annotation/IntrinsicCandidate.java
>
>Co-authored-by: Raffaello Giulietti 
>  - Shorter first sentence
>  - Updates, thanks to John
>  - Refine validation and defensive copying
>  - 8355223: Improve documentation on @IntrinsicCandidate

src/java.base/share/classes/jdk/internal/vm/annotation/IntrinsicCandidate.java 
line 39:

> 37:  * Intrinsification
> 38:  * The most frequently special treatment is intrinsification, which 
> replaces a
> 39:  * candidate method's body, bytecode or native, with handwritten platform

Is this sentence missing the word "code" after "native"? Should it have been:

> bytecode or native code, ...

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24777#discussion_r2097002980

Re: RFR: 8354724: Methods in java.io.Reader to read all characters and all lines [v22]

[Alan Bateman]

On Mon, 19 May 2025 06:59:58 GMT, Jaikiran Pai  wrote:

>>> I read this sentence a couple of times [...]
>> 
>> Please see 
>> [a86610d](https://github.com/openjdk/jdk/pull/24728/commits/a86610d06169445a5c4b81a0c60527130a45e045).
>> 
>>> I suspect that sentence was motivated from similar existing text in 
>>> `Files.readString(...)` and there too I find it confusing.
>> 
>> If you think it worthwhile, please file an issue to improve that verbiage as 
>> well.
>
> Thank you Brian, the updated text for this section looks good to me. Once 
> this PR gets integrated, I'll go through the Files.readXXX methods and file 
> an issue to have that text simplified too.

I see this has been changed to  "then some characters, but not all" but it 
doesn't flow very well. The original sentence, which was copied/modified from 
InputStream.readAllBytes, is much cleaner and I think I would prefer to go back 
to that.  Maybe Jai's issue could be address by dropping the first comma from 
the sentence?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24728#discussion_r2095001660

RFR: 8356995: Provide default methods min(T, T) and max(T, T) in Comparator interface

[Tagir F . Valeev]

Implementation of Comparator.min and Comparator.max methods. Preliminary 
discussion is in this thread:
https://mail.openjdk.org/pipermail/core-libs-dev/2025-May/145638.html
The specification is mostly composed of Math.min/max and Collections.min/max 
specifications. 

The methods are quite trivial, so I don't think we need more extensive testing 
(e.g., using different comparators). But if you have ideas of new useful tests, 
I'll gladly add them.

I'm not sure whether we should specify exactly the behavior in case if the 
comparator returns 0. I feel that it could be a useful invariant that 
`Comparator.min(a, b)` and `Comparator.max(a, b)` always return different 
argument, partitioning the set of {a, b} objects (even if they are equal). But 
I'm open to suggestions here.

-

Commit messages:
 - Fix linebreaks
 - JDK-8356995 Provide default methods min(T, T) and max(T, T) in Comparator 
interface

Changes: https://git.openjdk.org/jdk/pull/25297/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25297&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8356995
  Stats: 112 lines in 2 files changed: 110 ins; 1 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/25297.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25297/head:pull/25297

PR: https://git.openjdk.org/jdk/pull/25297

Re: RFR: 8354724: Methods in java.io.Reader to read all characters and all lines [v22]

[Jaikiran Pai]

On Fri, 16 May 2025 18:37:38 GMT, Brian Burkhalter  wrote:

>> src/java.base/share/classes/java/io/Reader.java line 478:
>> 
>>> 476:  *
>>> 477:  *  If an I/O error occurs reading from the stream, then it
>>> 478:  * may do so after some, but not all, characters have been read.
>> 
>> Hello Brian, I read this sentence a couple of times but it wasn't clear to 
>> me what this meant. I am guessing that we want to state that if an I/O error 
>> occurs when some (but not all?) characters have been read then the stream 
>> may be in an inconsistent state? Should we reword this paragraph?
>> 
>> I suspect that sentence was motivated from similar existing text in 
>> `Files.readString(...)` and there too I find it confusing.
>
>> I read this sentence a couple of times [...]
> 
> Please see 
> [a86610d](https://github.com/openjdk/jdk/pull/24728/commits/a86610d06169445a5c4b81a0c60527130a45e045).
> 
>> I suspect that sentence was motivated from similar existing text in 
>> `Files.readString(...)` and there too I find it confusing.
> 
> If you think it worthwhile, please file an issue to improve that verbiage as 
> well.

Thank you Brian, the updated text for this section looks good to me. Once this 
PR gets integrated, I'll go through the Files.readXXX methods and file an issue 
to have that text simplified too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24728#discussion_r2094977207

Integrated: 8357016: Candidate main methods not computed properly

[Jan Lahoda]

On Thu, 15 May 2025 16:42:48 GMT, Jan Lahoda  wrote:

> A consider class like this:
> 
> 
> public class TwoMains {
> private static void main(String... args) {}
> static void main() {
> System.out.println("Should be called, but is not.");
> }
> }
> 
> 
> The `MethodFinder` will do lookup for the `main(String[])` method, and it 
> finds one, so does not proceed with a lookup for `main()`. But then, it will 
> check the access modifier, and will reject that method, never going back to 
> the `main()` method. This is not what the JLS says about the lookup - the 
> private method is not a candidate, and should be ignored.
> 
> Something similar happens if the return type is not `void`.
> 
> This PR is fixing that by checking whether the `main(String[])` method is 
> usable early, and falling back to `main()` if it `main(String[])` is not 
> usable.
> 
> It also removes the check for the `abstract` method, as that, by itself, is 
> not really backed by JLS, but adds a check for `abstract` class, producing a 
> user-friendly message is trying to invoke an instance `main` method on an 
> `abstract` class (which, obviously, cannot be instantiated).

This pull request has now been integrated.

Changeset: 77a3e04f
Author:Jan Lahoda 
URL:   
https://git.openjdk.org/jdk/commit/77a3e04ffc27554c14e3d45ba16ad0ee8f3c1eb1
Stats: 177 lines in 8 files changed: 151 ins; 10 del; 16 mod

8357016: Candidate main methods not computed properly

Reviewed-by: jpai, vromero

-

PR: https://git.openjdk.org/jdk/pull/25256

Re: Towards a JSON API for the JDK

[Remi Forax]

- Original Message -
> From: "cay horstmann" 
> To: "core-libs-dev" 
> Sent: Sunday, May 18, 2025 7:55:12 AM
> Subject: Re: Towards a JSON API for the JDK

Hello Cay,

> +1 for having a JSON battery included with the JDK. And for "Our primary goal 
> is
> that the library be simple to use for parsing, traversing, and generating
> conformant JSON documents."
> 
> Generating JSON could be easier. Why not convenience methods Json.newObject 
> and
> Json.newArray like in https://github.com/arkanovicz/essential-json?
> 
> Parsing with instanceof will work, but is obviously painful today, as your
> example shows. The simplification with deconstruction patterns is not
> impressive either.
> 
> JsonValue doc = Json.parse(inputString);
> if (doc instanceof JsonObject(var members)
>  && members.get("name") instanceof JsonString(String name)
>  && members.get("age") instanceof JsonNumber(int age)) {
>  // use "name" and "age"
> } else throw new NoSuchArgumentException();
> 
> vs. Jackson
> 
> String name = doc.get("name").asText();
> int age = doc.get("age").asInt();
> ...
> 
> If only there was some deconstruction magic that approximates the JavaScript
> code
> 
> const doc = { name: "John", age: 30 }
> const { name, age } = doc

We already have that, it's called a record :)

Basically, you are advocating for a mapping JsonObject <--> record.

[...]

> 
> Cheers,
> 
> Cay
> 

Here is a simple JsonObject mapper using java.util.json types.
  https://github.com/forax/json-object-mapper

I believe this is also the fastest possible ... it leverages method handles to 
generate the mapping code (it's only 128 lines of code, obviously, it's a 
prototype)
  
https://github.com/forax/json-object-mapper/blob/master/src/main/java/json/RecordMapperImpl.java

It's mostly on par with hand-coded code (i've just a supplementary lookup 
through a StableValue)

  // BenchmarkMode  Cnt   Score   Error  Units
  // RecordMapperBench.mappingSimple  avgt5  15,057 ± 0,050  ns/op
  // RecordMapperBench.recordMapper   avgt5  16,997 ± 0,044  ns/op
  

regards,
Rémi

Re: RFR: 8354556: Expand value-based class warnings to java.lang.ref API [v16]

[Jan Lahoda]

On Thu, 15 May 2025 22:32:12 GMT, Vicente Romero  wrote:

>> This PR is defining a new internal annotation, 
>> `@jdk.internal.RequiresIdentity`, with target types PARAMETER and 
>> TYPE_PARAMETER. The @RequiresIdentity annotation expresses the expectation 
>> that an argument to a given method or constructor parameter will be an 
>> object with a unique identity, not an instance of a value-based class; or 
>> that the type argument to a given type parameter will not be a value-based 
>> class type.
>> 
>> For more details please refer to the complete description in the 
>> corresponding JIRA entry [1]
>> 
>> TIA
>> 
>> [1] https://bugs.openjdk.org/browse/JDK-8354556
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   addressing review comments

javac changes look good to me, with one trivial nit in `Check`.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 5848:

> 5846: }
> 5847: }
> 5848:  }

Nit - there appears to be an extra space at the beginning of this line.

-

Marked as reviewed by jlahoda (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24746#pullrequestreview-2849882159
PR Review Comment: https://git.openjdk.org/jdk/pull/24746#discussion_r2095145360

Re: RFR: 8356974: tools/launcher/ToolsOpts.java fails if the build id contains "-J"

[Jaikiran Pai]

On Wed, 14 May 2025 14:40:41 GMT, Manuel Hässig  wrote:

> When passing `-J-version` to the patched javac, 
> `tools/launcher/ToolsOpts.java` wants to verify that the output corresponds 
> to the expected version output. However, if the build id of the JDK running 
> this test contains the substring "-J", then the test fails incorrectly at:
> 
> https://github.com/openjdk/jdk/blob/97b0dd2167530b3d237e748cd5da0130e38e8af2/test/jdk/tools/launcher/ToolsOpts.java#L131-L134
> 
> This PR addresses this false positive by looking for the substring `" -J-"` 
> instead. The preceding space to ensure that `-J` occurs at the beginning of a 
> word as an argument would and a `-` suffix since `-J` options are always 
> followed by a dash. Further, this PR adds a print of the output of the test 
> result in case this condition fails, to be able to inspect what triggered the 
> failure.
> 
> Testing:
>  - [x] [Github 
> Actions](https://github.com/mhaessig/jdk/actions/runs/15023438332)
>  - [x] tier1 and tier2 for Oracle supported platforms and OSs plus Oracle 
> internal testing

Hello Manuel,

> When passing -J-version to the patched javac, tools/launcher/ToolsOpts.java 
> wants to verify that the output corresponds to the expected version output. 
> However, if the build id of the JDK running this test contains the substring 
> "-J", then the test fails incorrectly

The proposed change looks reasonable to me. A brief look through JEP-223 
https://openjdk.org/jeps/223 suggests that a space character cannot appear in 
the JDK version, so this proposed change to use " -J-" won't run into the same 
issue as previously.

The copyright year on this test will need an update. Please also add a 
"noreg-self" (https://openjdk.org/guide/#noreg) to the JBS issue.

-

PR Review: https://git.openjdk.org/jdk/pull/25230#pullrequestreview-2849969430

Re: RFR: 8356974: tools/launcher/ToolsOpts.java fails if the build id contains "-J" [v2]

[Jaikiran Pai]

On Mon, 19 May 2025 09:10:21 GMT, Manuel Hässig  wrote:

>> When passing `-J-version` to the patched javac, 
>> `tools/launcher/ToolsOpts.java` wants to verify that the output corresponds 
>> to the expected version output. However, if the build id of the JDK running 
>> this test contains the substring "-J", then the test fails incorrectly at:
>> 
>> https://github.com/openjdk/jdk/blob/97b0dd2167530b3d237e748cd5da0130e38e8af2/test/jdk/tools/launcher/ToolsOpts.java#L131-L134
>> 
>> This PR addresses this false positive by looking for the substring `" -J-"` 
>> instead. The preceding space to ensure that `-J` occurs at the beginning of 
>> a word as an argument would and a `-` suffix since `-J` options are always 
>> followed by a dash. Further, this PR adds a print of the output of the test 
>> result in case this condition fails, to be able to inspect what triggered 
>> the failure.
>> 
>> Testing:
>>  - [x] [Github 
>> Actions](https://github.com/mhaessig/jdk/actions/runs/15023438332)
>>  - [x] tier1 and tier2 for Oracle supported platforms and OSs plus Oracle 
>> internal testing
>
> Manuel Hässig has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update copyright

Thank you for the update. Looks good to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25230#pullrequestreview-2850018076

Re: RFR: 8356974: tools/launcher/ToolsOpts.java fails if the build id contains "-J" [v2]

[Manuel Hässig]

> When passing `-J-version` to the patched javac, 
> `tools/launcher/ToolsOpts.java` wants to verify that the output corresponds 
> to the expected version output. However, if the build id of the JDK running 
> this test contains the substring "-J", then the test fails incorrectly at:
> 
> https://github.com/openjdk/jdk/blob/97b0dd2167530b3d237e748cd5da0130e38e8af2/test/jdk/tools/launcher/ToolsOpts.java#L131-L134
> 
> This PR addresses this false positive by looking for the substring `" -J-"` 
> instead. The preceding space to ensure that `-J` occurs at the beginning of a 
> word as an argument would and a `-` suffix since `-J` options are always 
> followed by a dash. Further, this PR adds a print of the output of the test 
> result in case this condition fails, to be able to inspect what triggered the 
> failure.
> 
> Testing:
>  - [x] [Github 
> Actions](https://github.com/mhaessig/jdk/actions/runs/15023438332)
>  - [x] tier1 and tier2 for Oracle supported platforms and OSs plus Oracle 
> internal testing

Manuel Hässig has updated the pull request incrementally with one additional 
commit since the last revision:

  Update copyright

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/25230/files
  - new: https://git.openjdk.org/jdk/pull/25230/files/a11ff5e5..388f3683

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25230&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25230&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/25230.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25230/head:pull/25230

PR: https://git.openjdk.org/jdk/pull/25230

Re: RFR: 8357081: Removed unused methods of HexDigits

[Jaikiran Pai]

On Thu, 15 May 2025 22:03:22 GMT, Shaojin Wen  wrote:

> In HexDigits, getCharsLatin1 and getCharsUTF16 are no longer used, so remove 
> these methods

It does indeed look like these are unused. The change looks OK to me but please 
wait for one more review from someone more familiar with this code to 
understand if there's anything more to consider.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25258#pullrequestreview-2850079533

Re: RFR: 8357106: Add missing classpath exception copyright headers

[duke]

On Fri, 16 May 2025 07:04:04 GMT, Sorna Sarathi N  wrote:

> This PR adds missing classpath exception

@Sorna-Sarathi 
Your change (at version 1b55b287f84d5cad1958aec440f3b499a874b04c) is now ready 
to be sponsored by a Committer.

-

PR Comment: https://git.openjdk.org/jdk/pull/25261#issuecomment-2890316340

Re: RFR: 8356974: tools/launcher/ToolsOpts.java fails if the build id contains "-J" [v2]

[Manuel Hässig]

On Mon, 19 May 2025 09:13:14 GMT, Manuel Hässig  wrote:

>> When passing `-J-version` to the patched javac, 
>> `tools/launcher/ToolsOpts.java` wants to verify that the output corresponds 
>> to the expected version output. However, if the build id of the JDK running 
>> this test contains the substring "-J", then the test fails incorrectly at:
>> 
>> https://github.com/openjdk/jdk/blob/97b0dd2167530b3d237e748cd5da0130e38e8af2/test/jdk/tools/launcher/ToolsOpts.java#L131-L134
>> 
>> This PR addresses this false positive by looking for the substring `" -J-"` 
>> instead. The preceding space to ensure that `-J` occurs at the beginning of 
>> a word as an argument would and a `-` suffix since `-J` options are always 
>> followed by a dash. Further, this PR adds a print of the output of the test 
>> result in case this condition fails, to be able to inspect what triggered 
>> the failure.
>> 
>> Testing:
>>  - [x] [Github 
>> Actions](https://github.com/mhaessig/jdk/actions/runs/15023438332)
>>  - [x] tier1 and tier2 for Oracle supported platforms and OSs plus Oracle 
>> internal testing
>
> Manuel Hässig has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update copyright

Thank you for the reviews!

-

PR Comment: https://git.openjdk.org/jdk/pull/25230#issuecomment-2890310647

Re: RFR: 8357106: Add missing classpath exception copyright headers

[Jaikiran Pai]

On Fri, 16 May 2025 07:04:04 GMT, Sorna Sarathi N  wrote:

> This PR adds missing classpath exception

The change looks good to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25261#pullrequestreview-2850053263

Re: RFR: 8357178: Simplify Class::componentType [v2]

[Per Minborg]

On Mon, 19 May 2025 00:22:08 GMT, Chen Liang  wrote:

>> `isArray` and null return is now redundant when `componentType` is changed 
>> to an explicit field.
>
> Chen Liang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   For parity with MT.descriptorString

Question: Why was this overload added in the first place, as it seems 
equivalent to `getComponentType()`?

-

PR Comment: https://git.openjdk.org/jdk/pull/25280#issuecomment-2890245474

Re: RFR: 8077587: BigInteger Roots [v17]

[fabioromano1]

> This PR implements nth root computation for BigIntegers using Newton method.

fabioromano1 has updated the pull request incrementally with one additional 
commit since the last revision:

  Code simplification

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/24898/files
  - new: https://git.openjdk.org/jdk/pull/24898/files/307af7f6..fe82fa72

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=24898&range=16
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24898&range=15-16

  Stats: 3 lines in 1 file changed: 2 ins; 1 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/24898.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/24898/head:pull/24898

PR: https://git.openjdk.org/jdk/pull/24898

Integrated: 8356974: tools/launcher/ToolsOpts.java fails if the build id contains "-J"

[Manuel Hässig]

On Wed, 14 May 2025 14:40:41 GMT, Manuel Hässig  wrote:

> When passing `-J-version` to the patched javac, 
> `tools/launcher/ToolsOpts.java` wants to verify that the output corresponds 
> to the expected version output. However, if the build id of the JDK running 
> this test contains the substring "-J", then the test fails incorrectly at:
> 
> https://github.com/openjdk/jdk/blob/97b0dd2167530b3d237e748cd5da0130e38e8af2/test/jdk/tools/launcher/ToolsOpts.java#L131-L134
> 
> This PR addresses this false positive by looking for the substring `" -J-"` 
> instead. The preceding space to ensure that `-J` occurs at the beginning of a 
> word as an argument would and a `-` suffix since `-J` options are always 
> followed by a dash. Further, this PR adds a print of the output of the test 
> result in case this condition fails, to be able to inspect what triggered the 
> failure.
> 
> Testing:
>  - [x] [Github 
> Actions](https://github.com/mhaessig/jdk/actions/runs/15023438332)
>  - [x] tier1 and tier2 for Oracle supported platforms and OSs plus Oracle 
> internal testing

This pull request has now been integrated.

Changeset: 36c9be70
Author:Manuel Hässig 
Committer: Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/36c9be70e27eccdd2a156931fafa1f55dd3fb022
Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod

8356974: tools/launcher/ToolsOpts.java fails if the build id contains "-J"

Reviewed-by: jpai, thartmann

-

PR: https://git.openjdk.org/jdk/pull/25230

Re: RFR: 8356974: tools/launcher/ToolsOpts.java fails if the build id contains "-J" [v2]

[duke]

On Mon, 19 May 2025 09:13:14 GMT, Manuel Hässig  wrote:

>> When passing `-J-version` to the patched javac, 
>> `tools/launcher/ToolsOpts.java` wants to verify that the output corresponds 
>> to the expected version output. However, if the build id of the JDK running 
>> this test contains the substring "-J", then the test fails incorrectly at:
>> 
>> https://github.com/openjdk/jdk/blob/97b0dd2167530b3d237e748cd5da0130e38e8af2/test/jdk/tools/launcher/ToolsOpts.java#L131-L134
>> 
>> This PR addresses this false positive by looking for the substring `" -J-"` 
>> instead. The preceding space to ensure that `-J` occurs at the beginning of 
>> a word as an argument would and a `-` suffix since `-J` options are always 
>> followed by a dash. Further, this PR adds a print of the output of the test 
>> result in case this condition fails, to be able to inspect what triggered 
>> the failure.
>> 
>> Testing:
>>  - [x] [Github 
>> Actions](https://github.com/mhaessig/jdk/actions/runs/15023438332)
>>  - [x] tier1 and tier2 for Oracle supported platforms and OSs plus Oracle 
>> internal testing
>
> Manuel Hässig has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update copyright

@mhaessig 
Your change (at version 388f36832198c72268a3302cacb3192e9e93cf21) is now ready 
to be sponsored by a Committer.

-

PR Comment: https://git.openjdk.org/jdk/pull/25230#issuecomment-2890316802

Re: RFR: 8357178: Simplify Class::componentType [v2]

[Chen Liang]

On Mon, 19 May 2025 09:10:23 GMT, Per Minborg  wrote:

>> Chen Liang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   For parity with MT.descriptorString
>
> Question: Why was this overload added in the first place, as it seems 
> equivalent to `getComponentType()`?

@minborg It implements a method in TypeDescriptor.OfField.

-

PR Comment: https://git.openjdk.org/jdk/pull/25280#issuecomment-2890543283

Re: RFR: 8348986: Improve coverage of enhanced exception messages [v10]

[Daniel Fuchs]

On Fri, 16 May 2025 11:42:08 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> Enhanced exception messages are designed to hide sensitive information such 
>> as hostnames, IP 
>> addresses from exception message strings, unless the enhanced mode for the 
>> specific category 
>> has been explicitly enabled. Enhanced exceptions were first introduced in 
>> 8204233 in JDK 11 and 
>> updated in 8207846.
>> 
>> This PR aims to increase the coverage of enhanced exception messages in the 
>> networking code.
>> A limited number of exceptions are already hidden (restricted) by default. 
>> The new categories and 
>> exceptions in this PR will be restricted on an opt-in basis, ie. the default 
>> mode will be enhanced
>> (while preserving the existing behavior).
>> 
>> The mechanism is controlled by the security/system property 
>> "jdk.includeInExceptions" which takes as value
>> a comma separated list of category names, which identify groups of 
>> exceptions where the exception
>> message may be enhanced. Any category not listed is "restricted" which means 
>> that potentially
>> sensitive information (such as hostnames, IP addresses, user identities) are 
>> excluded from the message text.
>> 
>> The changes to the java.security conf file describe the exact changes in 
>> terms of the categories now
>> supported and any changes in behavior.
>> 
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 26 commits:
> 
>  - reduced number of new categories
>  - Merge branch 'master' into 8348986-exceptions
>  - Merge branch 'master' into 8348986-exceptions
>  - Merge branch 'master' into 8348986-exceptions
>  - Merge branch 'master' into 8348986-exceptions
>  - Review update
>  - review update
>  - Merge branch 'master' into 8348986-exceptions
>  - update to minimise code changes
>  - Merge branch 'master' into 8348986-exceptions
>  - ... and 16 more: https://git.openjdk.org/jdk/compare/64a858c7...3b7861b0

src/java.base/share/classes/jdk/internal/util/Exceptions.java line 78:

> 76:  * controlled. Consider using a unique value for the
> 77:  * SecurityProperties.includedInExceptions(String value) mechanism
> 78:  * Current values defined are "socket", "jar", "userInfo"

If I am not mistaken the "socket" info is no longer here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/23929#discussion_r2095457460

Re: RFR: 8355563: VectorAPI: Refactor current implementation of subword gather load API

[Jatin Bhateja]

On Mon, 19 May 2025 03:10:46 GMT, Xiaohong Gong  wrote:

>> JDK-8318650 introduced hotspot intrinsification of subword gather load APIs 
>> for X86 platforms [1]. However, the current implementation is not optimal 
>> for AArch64 SVE platform, which natively supports vector instructions for 
>> subword gather load operations using an int vector for indices (see [2][3]).
>> 
>> Two key areas require improvement:
>> 1. At the Java level, vector indices generated for range validation could be 
>> reused for the subsequent gather load operation on architectures with native 
>> vector instructions like AArch64 SVE. However, the current implementation 
>> prevents compiler reuse of these index vectors due to divergent control 
>> flow, potentially impacting performance.
>> 2. At the compiler IR level, the additional `offset` input for 
>> `LoadVectorGather`/`LoadVectorGatherMasked` with subword types  increases IR 
>> complexity and complicates backend implementation. Furthermore, generating 
>> `add` instructions before each memory access negatively impacts performance.
>> 
>> This patch refactors the implementation at both the Java level and compiler 
>> mid-end to improve efficiency and maintainability across different 
>> architectures.
>> 
>> Main changes:
>> 1. Java-side API refactoring:
>>- Explicitly passes generated index vectors to hotspot, eliminating 
>> duplicate index vectors for gather load instructions on
>>  architectures like AArch64.
>> 2. C2 compiler IR refactoring:
>>- Refactors `LoadVectorGather`/`LoadVectorGatherMasked` IR for subword 
>> types by removing the memory offset input and incorporating it into the 
>> memory base `addr` at the IR level. This simplifies backend implementation, 
>> reduces add operations, and unifies the IR across all types.
>> 3. Backend changes:
>>- Streamlines X86 implementation of subword gather operations following 
>> the removal of the offset input from the IR level.
>> 
>> Performance:
>> The performance of the relative JMH improves up to 27% on a X86 AVX512 
>> system. Please see the data below:
>> 
>> Benchmark Mode   Cnt Unit
>> SIZEBefore  AfterGain
>> GatherOperationsBenchmark.microByteGather128  thrpt  30  ops/ms  
>> 6453682.012   52650.325  0.98
>> GatherOperationsBenchmark.microByteGather128  thrpt  30  ops/ms  
>> 256   14484.252   14255.156  0.98
>> GatherOperationsBenchmark.microByteGather128  thrpt  30  ops/ms  
>> 1024   3664.9003595.615  0.98
>> GatherOperationsBenchmark.microByteGather128  thrpt  30  ops/ms  
>> 4096908.31...
>
> Ping again~  could any one please take a look at this PR？ Thanks a lot!

Hi @XiaohongGong ,
Very nice work!,  Looks good to me, will do some testing and get back.

Do you have any idea about following regression?


GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  64 
   55844.814   48311.847  0.86
GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
256   15139.459   13009.848  0.85
GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
1024   3861.8343284.944  0.85
GatherOperationsBenchmark.microByteGather256  thrpt  30  ops/ms  
4096938.665 817.673  0.87


Best Regards

-

PR Comment: https://git.openjdk.org/jdk/pull/25138#issuecomment-2890659302

RFR: 8356629: Incorrect use of {@linkplain} in java.sql

[Nizar Benalla]

Please review this trivial patch to fix a javadoc bug.

TIA

-

Commit messages:
 - use  PseudoColumnUsage#name() instead
 - fix incorrect `@link` tags

Changes: https://git.openjdk.org/jdk/pull/25286/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25286&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8356629
  Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/25286.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25286/head:pull/25286

PR: https://git.openjdk.org/jdk/pull/25286

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[David Beaumont]

On Fri, 16 May 2025 15:38:33 GMT, Lance Andersen  wrote:

>> src/jdk.zipfs/share/classes/module-info.java line 299:
>> 
>>> 297:  *   
>>> 298:  *   Any other values will cause an {@code 
>>> IllegalArgumentException}
>>> 299:  *   to be thrown.
>> 
>> The wording looks great. Just one thing with the "causes an IAE to be 
>> thrown" where I think it can be expanded to say that it causes IAE to be 
>> thrown when attempting to create the ZIP file system. The existing 
>> compressMethod property has word for this too.
>
> The IAE message would be best to standardize around the wording that the 
> compressMethod property uses (same for releaseVersion while we are at it.

Done.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095504122

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[David Beaumont]

On Fri, 16 May 2025 15:19:26 GMT, Lance Andersen  wrote:

>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 230:
>> 
>>> 228: // It requires 'entryLookup' and 'readOnly' to have safe 
>>> defaults (which
>>> 229: // is why they are the only non-final fields), and it requires 
>>> that the
>>> 230: // inode map has been initialized.
>> 
>> It's good to note that `determineReleaseVersion(...)` (and 
>> `createVersionedLinks(...)`) access instance fields of the `ZipFileSystem` 
>> being constructed. I think the comment however could be brief and should 
>> leave out the details about safe defaults.
>> 
>> Perhaps something like:
>> 
>>> determineReleaseVersion() and createVersionedLinks() access instance fields 
>>> while 'this' ZipFileSystem instance is being constructed.
>
> Not sure I see a need for the last sentence regarding the inode map having to 
> be initialized in addition to Jai's comments above

Fair enough, removed. I err on the side of over explaining things for future 
maintainers.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095518822

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[David Beaumont]

On Wed, 14 May 2025 16:42:07 GMT, Lance Andersen  wrote:

>> David Beaumont has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Changes based on review feedback.
>
> test/jdk/jdk/nio/zipfs/TestPosix.java line 434:
> 
>> 432: createTestZipFile(ZIP_FILE, ENV_DEFAULT).close();
>> 433: // check entries on zipfs with default options
>> 434: try (FileSystem zip = FileSystems.newFileSystem(ZIP_FILE, 
>> ENV_DEFAULT)) {
> 
> This tests an empty Map and should still be a test as it is different from 
> ENV_READ_ONLY

Revert and added new ReadOnly variants of those tests. Thanks for calling me 
out on this, it was a bit sloppy of me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095503763

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[David Beaumont]

On Fri, 16 May 2025 15:17:40 GMT, Lance Andersen  wrote:

>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 221:
>> 
>>> 219: }
>>> 220: // sm and existence check
>>> 221: zfpath.getFileSystem().provider().checkAccess(zfpath, 
>>> java.nio.file.AccessMode.READ);
>> 
>> Type name clash with existing AccessMode class. Since new enum is private, 
>> happy to rename.
>
> So perhaps change the name of the newly added enum

Done.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095527265

Re: RFR: 8354556: Expand value-based class warnings to java.lang.ref API [v16]

[Johannes Döbler]

On Thu, 15 May 2025 22:32:12 GMT, Vicente Romero  wrote:

>> This PR is defining a new internal annotation, 
>> `@jdk.internal.RequiresIdentity`, with target types PARAMETER and 
>> TYPE_PARAMETER. The @RequiresIdentity annotation expresses the expectation 
>> that an argument to a given method or constructor parameter will be an 
>> object with a unique identity, not an instance of a value-based class; or 
>> that the type argument to a given type parameter will not be a value-based 
>> class type.
>> 
>> For more details please refer to the complete description in the 
>> corresponding JIRA entry [1]
>> 
>> TIA
>> 
>> [1] https://bugs.openjdk.org/browse/JDK-8354556
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   addressing review comments

make/langtools/src/classes/build/tools/symbolgenerator/CreateSymbols.java line 
308:

> 306: "Ljdk/internal/ValueBased+Annotation;";
> 307: private static final String REQUIRES_IDENTITY_ANNOTATION =
> 308: "Ljdk/internal/RequieresIdentity;";

typo: RequieresIdentity -> RequiresIdentity

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24746#discussion_r2095534505

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[David Beaumont]

On Fri, 16 May 2025 14:23:38 GMT, Jaikiran Pai  wrote:

>> David Beaumont has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Changes based on review feedback.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 114:
> 
>> 112: private final ZipPath rootdir;
>> 113: // Starts in readOnly (safe mode), but might be reset at the end of 
>> initialization.
>> 114: private boolean readOnly = true;
> 
> If `readOnly` gets used by some code when a `ZipFileSystem` instance is being 
> constructed (which is why I believe this can't be a `final` field), then I 
> think we should not change this value to `true`. In other words, would this 
> change now have a chance of introducing a `ReadOnlyFileSystemException` when 
> constructing the `ZipFileSystem` whereas before it wouldn't?

""would this change now have a chance of introducing a 
ReadOnlyFileSystemException when constructing the ZipFileSystem whereas before 
it wouldn't?""

If there was ever a "ReadOnlyFileSystemException" when initializing the 
ZipFileSystem, we have a very serious bug already. Since we already have the 
opening of Zip working for cases where the underlying file is read-only in the 
file system, it has to be true that no attempt is made to modify the file 
contents.

While I accept that this new code now calls out to functions with this flag in 
a different state to before, I believe this is an absolute improvement since:

1. It is not acceptable to say to users "we support a read-only mode" if during 
initialization we might modify the file in any way (even including changing 
last-modification times etc.).

2. All the evidence points to whichever operations are being done during init 
as being fundamentally "read-only" (both due to it working on read-only zip 
files, and there being no conceptual need to modify anything during setup).

I'll happily do a thorough audit of everything which could be affected by this 
change if that will give you confidence, but I would not want to change this 
code back to its default "read-write until stated otherwise" behaviour.

> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 245:
> 
>> 243: : "The underlying ZIP file is not writable";
>> 244: throw new IOException(
>> 245: "A writable ZIP file system could not be opened 
>> for: " + zfpath + "\n" + reason);
> 
> The JDK coding guidelines recommend excluding file paths from exception 
> messages. So in this case the `zfpath` should be left out from the exception 
> message. Additionally, I haven't seen us using newline characters in 
> exception messages that we construct in the JDK code. So I think we should 
> leave that out too. 
> 
> Given this, I think it might be simpler to just change this `if` block to 
> something like:
> 
> 
> 
> if (...) {
> String reason = multiReleaseVersion.isPresent()
> ? "the multi-release JAR file is not writable"
> : "the ZIP file is not writable";
> 
> throw new IOException(reason);
> }

Thanks for letting me know. Now I think about the "no filename" things is very 
sensible for core libraries.
Done.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095517691
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095520598

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[David Beaumont]

On Fri, 16 May 2025 15:35:18 GMT, Lance Andersen  wrote:

>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 259:
>> 
>>> 257: 
>>> 258: // Pass "this" as a parameter after everything else is set up.
>>> 259: this.rootdir = new ZipPath(this, new byte[]{'/'});
>> 
>> This could probably be set above release version etc. but it's a choice of 
>> either:
>> 1. waiting until everything is set up before passing "this"
>> 2. letting an incomplete "this" instance get passed to another class 
>> (possible escape risk?)
>> 
>> Though in this case the "incompleteness" is limited to it not being set up 
>> for multi-jar reading yet, which absolutely shouldn't affect the root path. 
>> It feels safer to me to leave it to the end, or perhaps we should dig in to 
>> ZipPath, figure out what it really wants here, and just call a different 
>> constructor?
>
> I think it was fine where it was originally given how rootdir is used

I'd rather at least have it over the last part where the non-final fields are 
modified (in that state it's an instance functionally identical to one opened 
on a non-MR JAR).

Looking through the ZipPath code, it seems to (currently) only need to read 
back the ZipCoder field (but I'm still nervous about restoring the "this" 
escape to its original location since the ZipPath code might change one day and 
rely on something not initialized).

I moved it so it's the last final field initialized, but I'll revert it 
completely if you want.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095549535

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[David Beaumont]

On Fri, 16 May 2025 14:37:28 GMT, Jaikiran Pai  wrote:

>> David Beaumont has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Changes based on review feedback.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 241:
> 
>> 239: this.readOnly = forceReadOnly || 
>> multiReleaseVersion.isPresent() || !Files.isWritable(zfpath);
>> 240: if (readOnly && accessMode == AccessMode.READ_WRITE) {
>> 241: String reason = Files.isWritable(zfpath)
> 
> Nit - this additional call to Files.isWritable(...) can be avoided if we 
> store the value of the previous call (a couple of lines above). I realize 
> that the previous `Files.isWritable` is stashed at the end of the `||` 
> conditionals to prevent it from being invoked in certain situations.
> 
> So maybe a better change would be something like:
> 
> 
> String reason = multiReleaseVersion.isPresent()
>? "A multi-release JAR file opened with a specified version is not 
> writable"
>: "The underlying ZIP file is not writable";
> 
> 
> which would then avoid any additional calls to `Files.isWritable`.

Done.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095532769

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[David Beaumont]

> Adding read-only support to ZipFileSystem.
> 
> The new `accessMode` environment property allows for readOnly and readWrite 
> values, and ensures that the requested mode is consistent with what's 
> returned.
> 
> This involved a little refactoring to ensure that "read only" state was set 
> initially and only unset at the end of initialization if appropriate.
> 
> By making 2 methods return values (rather than silently set non-final fields 
> as a side effect) it's now clear in what order fields are initialized and 
> which are final (sadly there are still non-final fields, but only a split of 
> this class into two types can fix that, since determining multi-jar support 
> requires reading the file system).

David Beaumont has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed test.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/25178/files
  - new: https://git.openjdk.org/jdk/pull/25178/files/a82e4019..ea781c16

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=04-05

  Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/25178.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25178/head:pull/25178

PR: https://git.openjdk.org/jdk/pull/25178

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[Jaikiran Pai]

On Mon, 19 May 2025 11:41:16 GMT, David Beaumont  wrote:

>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 114:
>> 
>>> 112: private final ZipPath rootdir;
>>> 113: // Starts in readOnly (safe mode), but might be reset at the end 
>>> of initialization.
>>> 114: private boolean readOnly = true;
>> 
>> If `readOnly` gets used by some code when a `ZipFileSystem` instance is 
>> being constructed (which is why I believe this can't be a `final` field), 
>> then I think we should not change this value to `true`. In other words, 
>> would this change now have a chance of introducing a 
>> `ReadOnlyFileSystemException` when constructing the `ZipFileSystem` whereas 
>> before it wouldn't?
>
> ""would this change now have a chance of introducing a 
> ReadOnlyFileSystemException when constructing the ZipFileSystem whereas 
> before it wouldn't?""
> 
> If there was ever a "ReadOnlyFileSystemException" when initializing the 
> ZipFileSystem, we have a very serious bug already. Since we already have the 
> opening of Zip working for cases where the underlying file is read-only in 
> the file system, it has to be true that no attempt is made to modify the file 
> contents.
> 
> While I accept that this new code now calls out to functions with this flag 
> in a different state to before, I believe this is an absolute improvement 
> since:
> 
> 1. It is not acceptable to say to users "we support a read-only mode" if 
> during initialization we might modify the file in any way (even including 
> changing last-modification times etc.).
> 
> 2. All the evidence points to whichever operations are being done during init 
> as being fundamentally "read-only" (both due to it working on read-only zip 
> files, and there being no conceptual need to modify anything during setup).
> 
> I'll happily do a thorough audit of everything which could be affected by 
> this change if that will give you confidence, but I would not want to change 
> this code back to its default "read-write until stated otherwise" behaviour.

Hello David, I had another look at this code and after going through it, it 
looked like `readOnly` field can in fact be made `final` because of the 
refactoring changes that you did in this PR. I checked out your latest PR 
locally and here's the additional change I did:


diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java 
b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
index e2fddd96fe8..f54b5360ac5 100644
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
@@ -110,8 +110,7 @@ class ZipFileSystem extends FileSystem {
 private final Path zfpath;
 final ZipCoder zc;
 private final ZipPath rootdir;
-// Starts in readOnly (safe mode), but might be reset at the end of 
initialization.
-private boolean readOnly = true;
+private final boolean readOnly;
 
 // default time stamp for pseudo entries
 private final long zfsDefaultTimeStamp = System.currentTimeMillis();
@@ -227,11 +226,6 @@ static ZipAccessMode from(Object value) {
 
 // Determining a release version uses 'this' instance to read paths 
etc.
 Optional multiReleaseVersion = determineReleaseVersion(env);
-
-// Set the version-based lookup function for multi-release JARs.
-this.entryLookup =
-
multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
-
 // We only allow read-write zip/jar files if they are not multi-release
 // JARs and the underlying file is writable.
 this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() || 
!Files.isWritable(zfpath);
@@ -241,6 +235,10 @@ static ZipAccessMode from(Object value) {
 : "the ZIP file is not writable";
 throw new IOException(reason);
 }
+// Set the version-based lookup function for multi-release JARs.
+this.entryLookup =
+
multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
+
 }
 
 /**




With the refactoring changes you have done so far, we are now able to determine 
the ultimate value for `readOnly` before anything in the construction of 
`ZipFileSystem` would need access to it. Does this additional change look 
reasonable to you? I haven't run any tests against this change.

Making it `final` and having it not accessed until the ultimate value is set in 
the constructor would get us past any of the concerns we may have about the 
"initial" value of `readOnly`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r209558

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v4]

[Jaikiran Pai]

On Mon, 19 May 2025 12:22:11 GMT, Jaikiran Pai  wrote:

>> ""would this change now have a chance of introducing a 
>> ReadOnlyFileSystemException when constructing the ZipFileSystem whereas 
>> before it wouldn't?""
>> 
>> If there was ever a "ReadOnlyFileSystemException" when initializing the 
>> ZipFileSystem, we have a very serious bug already. Since we already have the 
>> opening of Zip working for cases where the underlying file is read-only in 
>> the file system, it has to be true that no attempt is made to modify the 
>> file contents.
>> 
>> While I accept that this new code now calls out to functions with this flag 
>> in a different state to before, I believe this is an absolute improvement 
>> since:
>> 
>> 1. It is not acceptable to say to users "we support a read-only mode" if 
>> during initialization we might modify the file in any way (even including 
>> changing last-modification times etc.).
>> 
>> 2. All the evidence points to whichever operations are being done during 
>> init as being fundamentally "read-only" (both due to it working on read-only 
>> zip files, and there being no conceptual need to modify anything during 
>> setup).
>> 
>> I'll happily do a thorough audit of everything which could be affected by 
>> this change if that will give you confidence, but I would not want to change 
>> this code back to its default "read-write until stated otherwise" behaviour.
>
> Hello David, I had another look at this code and after going through it, it 
> looked like `readOnly` field can in fact be made `final` because of the 
> refactoring changes that you did in this PR. I checked out your latest PR 
> locally and here's the additional change I did:
> 
> 
> diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java 
> b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
> index e2fddd96fe8..f54b5360ac5 100644
> --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
> +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
> @@ -110,8 +110,7 @@ class ZipFileSystem extends FileSystem {
>  private final Path zfpath;
>  final ZipCoder zc;
>  private final ZipPath rootdir;
> -// Starts in readOnly (safe mode), but might be reset at the end of 
> initialization.
> -private boolean readOnly = true;
> +private final boolean readOnly;
>  
>  // default time stamp for pseudo entries
>  private final long zfsDefaultTimeStamp = System.currentTimeMillis();
> @@ -227,11 +226,6 @@ static ZipAccessMode from(Object value) {
>  
>  // Determining a release version uses 'this' instance to read paths 
> etc.
>  Optional multiReleaseVersion = determineReleaseVersion(env);
> -
> -// Set the version-based lookup function for multi-release JARs.
> -this.entryLookup =
> -
> multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
> -
>  // We only allow read-write zip/jar files if they are not 
> multi-release
>  // JARs and the underlying file is writable.
>  this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() || 
> !Files.isWritable(zfpath);
> @@ -241,6 +235,10 @@ static ZipAccessMode from(Object value) {
>  : "the ZIP file is not writable";
>  throw new IOException(reason);
>  }
> +// Set the version-based lookup function for multi-release JARs.
> +this.entryLookup =
> +
> multiReleaseVersion.map(this::createVersionedLinks).orElse(Function.identity());
> +
>  }
>  
>  /**
> 
> 
> 
> 
> With the refactoring changes you have done so far, we are now able to 
> determine the ultimate value for `readOnly` before anything in the 
> construction of `ZipFileSystem` would need access to it. Does this additional 
> change look reasonable to you? I haven't run any tests against this change.
> 
> Making it `final` and having it not accessed until the ultimate value is set 
> in the constructor would get us past any of t...

On second thought, may be this isn't enough. I see that I missed the fact that 
`determineReleaseVersion()` requires access to `this`. Please leave this in the 
current form that you have in your PR. I need a few more moments to consider if 
anything needs to be changed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095591964

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 12:15:38 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed test.

src/jdk.zipfs/share/classes/module-info.java line 290:

> 288:  *   already exist, and is incompatible with {@code 
> "create"=true}.
> 289:  *   Specifying both will cause an {@code 
> IllegalArgumentException}
> 290:  *   to be thrown when the Zip filesystem is created

To be precise, perhaps this last sentence should be reworded to:

> Specifying {@code create} as {@code true} and {@code accessMode} as {@code 
> readOnly} will cause an {@code IllegalArgumentException} to be thrown when 
> the ZIP filesystem is created.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095608509

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 12:15:38 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed test.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 210:

> 208: }
> 209: }
> 210: // sm and existence check

Nit - this is pre-existing, but would be good to cleanup. The "sm" here refers 
to SecurityManager. In JDK mainline, the SecurityManager is no longer present. 
So it would be good to change this comment to just "existence check" and remove 
reference to "sm".

src/jdk.zipfs/share/classes/module-info.java line 304:

> 302:  *   
> 303:  *   
> 304:  *   The access mode has no effect on reported POSIX file 
> permissions (in cases

For consistency, I think this should be:

> The {@code accessMode} has no 

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095621216
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095616802

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 12:15:38 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed test.

test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 224:

> 222: // Underlying file is read-only.
> 223: Path readOnlyZip = Utils.createJarFile("read_only.zip", 
> Map.of("file.txt", "Hello World"));
> 224: readOnlyZip.toFile().setReadOnly();

`java.io.File.setReadOnly()` specifies:

> On some platforms it may be possible to start the
> Java virtual machine with special privileges that allow it to modify
> files that are marked read-only. Whether or not a read-only file or
> directory may be deleted depends upon the underlying system.

So I think we should run the subsequent asserts in this test after first 
checking if the file was set to read-only. If it isn't then we should skip the 
test. Something like:


final boolean marked  = readOnlyZip.toFile().setReadOnly();
Assumptions.assumeTrue(marked, "skipping test since " + readOnlyZip + " 
couldn't be marked read-only");
assertThrows(IOException.class,
() -> FileSystems.newFileSystem(readOnlyZip, 
Map.of("accessMode", "readWrite")));

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095650541

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v3]

[Jaikiran Pai]

On Thu, 15 May 2025 17:27:42 GMT, David Beaumont  wrote:

>> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 167:
>> 
>>> 165: return null;
>>> 166: }
>>> 167: case String label when READ_WRITE.label.equals(label) 
>>> -> {
>> 
>> I haven't yet fully caught up on the newer features of switch/case. Does 
>> this have any advantage as compared to the simpler:
>> 
>> 
>> case "readWrite" -> {
>>return READ_WRITE;
>> }
>> case "readOnly" -> {
>>return READ_ONLY;
>> }
>
> Or just an if :)
> 
> The reason for the new style was the "need" for the "String label when" 
> qualifier, which I don't actually need because String.equals(xx) copes with 
> being given non-strings fine. You can't use the syntax you suggested in the 
> new switch since "value" isn't a String.

Thank you for updating this to a traditional and trivial `if/else`.

>> src/jdk.zipfs/share/classes/module-info.java line 277:
>> 
>>> 275:  *   
>>> 276:  *   A value defining the desired read/write access mode of the 
>>> file system
>>> 277:  *   (either read-write or read-only).
>> 
>> This line is slightly confusing. Initially I thought `read-write` and 
>> `read-only` are the actual values that this environment property will 
>> accept. But further review suggests that the actual literals supported are 
>> `readOnly` and `readWrite` and this line here I think is trying to explain 
>> the supported semantics of the file system.
>> 
>> Perhaps we could reword this to something like:
>> 
>>>
>>> A value defining the desired access mode of the file system. A ZIP file 
>>> system can be created to allow for read-write access or read-only access.
>
> Yeah, I tried to distinguish the accessMode flags (for which there are 3 
> states, ro, rw, n/a) and the final state of the file system from 
> fs.isReadOnly(). Hence using `...` for whenever the actually 
> resulting state is mentioned. I guess it wasn't clear enough.

Thank you for updating this part, this is much more clearer now.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095625151
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095622485

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 12:15:38 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed test.

test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 208:

> 206: // Multi-release JARs, when opened with a specified version are 
> inherently read-only.
> 207: Path multiReleaseJar = createMultiReleaseJar();
> 208: try (FileSystem fs = FileSystems.newFileSystem(multiReleaseJar, 
> Map.of("accessMode", "readWrite"))) {

I wonder if the ZIP filesystem implementation should throw an exception in this 
case. In the case where the application code has explicitly stated 
`accessMode=readWrite`, if the underlying file `Path` isn't writable, then we 
currently throw an `IOException`. I think we should specify and implement the 
same for the case where `accessMode=readWrite` and the file is a multi-release 
JAR file.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095637058

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 12:15:38 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed test.

On a general note, all of these updated files will require a copyright year 
update.

test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 239:

> 237: assertTrue(fs.isReadOnly());
> 238: if (!"First 
> version".equals(Files.readString(fs.getPath("file.txt"), UTF_8))) {
> 239: fail("unexpected file content");

Nit, for this and the other new `fail` statements, it might be good to include 
the unexpected file content in the fail message. If at all the test fails for 
whatever reason, that additional detail usually help to quickly debug the 
failures. Or maybe just replace the `if` followed by a `fail` with an 
`assertEquals()` call.

-

PR Comment: https://git.openjdk.org/jdk/pull/25178#issuecomment-2890930980
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095657482

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 12:15:38 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed test.

test/jdk/jdk/nio/zipfs/Utils.java line 52:

> 50:  */
> 51: static Path createJarFile(String name, String... entries) throws 
> IOException {
> 52: Path jarFile = Paths.get(name);

In recent times we have replaced `Paths.get(...)` call in the JDK code with 
`Path.of(...)`. We should do the same here and the other line in this utility 
class.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095676145

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 12 May 2025 10:01:05 GMT, David Beaumont  wrote:

>> David Beaumont has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fixed test.
>
> test/jdk/jdk/nio/zipfs/Utils.java line 52:
> 
>> 50:  */
>> 51: static Path createJarFile(String name, String... entries) throws 
>> IOException {
>> 52: Path jarFile = Paths.get(name);
> 
> Previous tests always had the test file called the same thing.
> Note that in jtreg tests, the file of the same name often already exists in 
> the test directory.
> I tried adding a "ensure file doesn't already exist" check and it fails lots 
> of tests.

It's interesting that we have several places in the test which use this 
`Utils.createJarFile()` and pass it a test specific JAR file name and yet this 
utility was ignoring the passed name hard coding the name to `basic.jar`. It 
appears to be an oversight and your fix here I believe is a good thing.

> I tried adding a "ensure file doesn't already exist" check and it fails lots 
> of tests.

I think it's OK in its current form to allow the file to be overwritten. Plus, 
you have also updated the javadoc of this test utility method to explicitly 
state its current behaviour, which I think is enough to let the call sites know 
how this behaves.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095672596

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 12:15:38 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed test.

test/jdk/jdk/nio/zipfs/Utils.java line 48:

> 46:  *
> 47:  * @param name the file name of the jar file to create in the working 
> directory.
> 48:  * @param entries a list of JAR entries to be populated with random 
> bytes.

Nit - maybe reword this to:

> @param entries JAR file entry names, whose content will be populated with 
> random bytes.

test/jdk/jdk/nio/zipfs/Utils.java line 78:

> 76:  *
> 77:  * @param name the file name of the jar file to create in the working 
> directory.
> 78:  * @param entries a map of relative file name path strings to file 
> content

Nit - I think we should replace this description with something like:

> @param entries a map of JAR file entry names to entry content

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095686442
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095684096

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 12:15:38 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed test.

test/jdk/jdk/nio/zipfs/TestPosix.java line 2:

> 1: /*
> 2:  * Copyright (c) 2019, 2025, SAP SE. All rights reserved.

For non-Oracle copyright lines like these, we don't edit them and instead we 
introduce a newline for the Oracle copyright year. So we should revert the 
change to this line and introduce the following new line just after this 
current one:

* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095707195

Re: RFR: 8357081: Removed unused methods of HexDigits

[Shaojin Wen]

On Thu, 15 May 2025 22:03:22 GMT, Shaojin Wen  wrote:

> In HexDigits, getCharsLatin1 and getCharsUTF16 are no longer used, so remove 
> these methods

The getCharsLatin1/getCharsUTF16 methods of HexDigits may be used in 
j.u.Formatter/HexFormat in the future. For this reason, should we keep them? 
@cl4es

-

PR Comment: https://git.openjdk.org/jdk/pull/25258#issuecomment-2891047804

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[David Beaumont]

On Mon, 19 May 2025 12:39:22 GMT, Jaikiran Pai  wrote:

>> David Beaumont has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fixed test.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 210:
> 
>> 208: }
>> 209: }
>> 210: // sm and existence check
> 
> Nit - this is pre-existing, but would be good to cleanup. The "sm" here 
> refers to SecurityManager. In JDK mainline, the SecurityManager is no longer 
> present. So it would be good to change this comment to just "existence check" 
> and remove reference to "sm".

Done.

> src/jdk.zipfs/share/classes/module-info.java line 290:
> 
>> 288:  *   already exist, and is incompatible with {@code 
>> "create"=true}.
>> 289:  *   Specifying both will cause an {@code 
>> IllegalArgumentException}
>> 290:  *   to be thrown when the Zip filesystem is created
> 
> To be precise, perhaps this last sentence should be reworded to:
> 
>> Specifying {@code create} as {@code true} and {@code accessMode} as {@code 
>> readOnly} will cause an {@code IllegalArgumentException} to be thrown when 
>> the ZIP filesystem is created.

Done.

> src/jdk.zipfs/share/classes/module-info.java line 304:
> 
>> 302:  *   
>> 303:  *   
>> 304:  *   The access mode has no effect on reported POSIX file 
>> permissions (in cases
> 
> For consistency, I think this should be:
> 
>> The {@code accessMode} has no 

Done.

> test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 208:
> 
>> 206: // Multi-release JARs, when opened with a specified version are 
>> inherently read-only.
>> 207: Path multiReleaseJar = createMultiReleaseJar();
>> 208: try (FileSystem fs = FileSystems.newFileSystem(multiReleaseJar, 
>> Map.of("accessMode", "readWrite"))) {
> 
> I wonder if the ZIP filesystem implementation should throw an exception in 
> this case. In the case where the application code has explicitly stated 
> `accessMode=readWrite`, if the underlying file `Path` isn't writable, then we 
> currently throw an `IOException`. I think we should specify and implement the 
> same for the case where `accessMode=readWrite` and the file is a 
> multi-release JAR file.

It does throw if accessMode=readWrite and the result is not writable for any 
reason.


this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() || 
!Files.isWritable(zfpath);
if (readOnly && accessMode == ZipAccessMode.READ_WRITE) {

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095718428
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095714913
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095715684
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095723172

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v7]

[David Beaumont]

> Adding read-only support to ZipFileSystem.
> 
> The new `accessMode` environment property allows for readOnly and readWrite 
> values, and ensures that the requested mode is consistent with what's 
> returned.
> 
> This involved a little refactoring to ensure that "read only" state was set 
> initially and only unset at the end of initialization if appropriate.
> 
> By making 2 methods return values (rather than silently set non-final fields 
> as a side effect) it's now clear in what order fields are initialized and 
> which are final (sadly there are still non-final fields, but only a split of 
> this class into two types can fix that, since determining multi-jar support 
> requires reading the file system).

David Beaumont has updated the pull request incrementally with one additional 
commit since the last revision:

  Changes based on feedback

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/25178/files
  - new: https://git.openjdk.org/jdk/pull/25178/files/ea781c16..46b19106

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=05-06

  Stats: 26 lines in 5 files changed: 6 ins; 1 del; 19 mod
  Patch: https://git.openjdk.org/jdk/pull/25178.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25178/head:pull/25178

PR: https://git.openjdk.org/jdk/pull/25178

Re: RFR: 8354556: Expand value-based class warnings to java.lang.ref API [v17]

[Vicente Romero]

> This PR is defining a new internal annotation, 
> `@jdk.internal.RequiresIdentity`, with target types PARAMETER and 
> TYPE_PARAMETER. The @RequiresIdentity annotation expresses the expectation 
> that an argument to a given method or constructor parameter will be an object 
> with a unique identity, not an instance of a value-based class; or that the 
> type argument to a given type parameter will not be a value-based class type.
> 
> For more details please refer to the complete description in the 
> corresponding JIRA entry [1]
> 
> TIA
> 
> [1] https://bugs.openjdk.org/browse/JDK-8354556

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/24746/files
  - new: https://git.openjdk.org/jdk/pull/24746/files/def2505f..cf92614c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=24746&range=16
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24746&range=15-16

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/24746.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/24746/head:pull/24746

PR: https://git.openjdk.org/jdk/pull/24746

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[David Beaumont]

On Mon, 19 May 2025 12:58:00 GMT, Jaikiran Pai  wrote:

>> David Beaumont has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fixed test.
>
> test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 239:
> 
>> 237: assertTrue(fs.isReadOnly());
>> 238: if (!"First 
>> version".equals(Files.readString(fs.getPath("file.txt"), UTF_8))) {
>> 239: fail("unexpected file content");
> 
> Nit, for this and the other new `fail` statements, it might be good to 
> include the unexpected file content in the fail message. If at all the test 
> fails for whatever reason, that additional detail usually help to quickly 
> debug the failures. Or maybe just replace the `if` followed by a `fail` with 
> an `assertEquals()` call.

Urgh, my bad. That's the hangover from these tests being first implemented in 
the ZipFSTester class without access to sensible assert methods.

> test/jdk/jdk/nio/zipfs/TestPosix.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2019, 2025, SAP SE. All rights reserved.
> 
> For non-Oracle copyright lines like these, we don't edit them and instead we 
> introduce a newline for the Oracle copyright year. So we should revert the 
> change to this line and introduce the following new line just after this 
> current one:
> 
> * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

Thanks! Done.

> test/jdk/jdk/nio/zipfs/Utils.java line 48:
> 
>> 46:  *
>> 47:  * @param name the file name of the jar file to create in the 
>> working directory.
>> 48:  * @param entries a list of JAR entries to be populated with random 
>> bytes.
> 
> Nit - maybe reword this to:
> 
>> @param entries JAR file entry names, whose content will be populated with 
>> random bytes.

Done.

> test/jdk/jdk/nio/zipfs/Utils.java line 52:
> 
>> 50:  */
>> 51: static Path createJarFile(String name, String... entries) throws 
>> IOException {
>> 52: Path jarFile = Paths.get(name);
> 
> In recent times we have replaced `Paths.get(...)` call in the JDK code with 
> `Path.of(...)`. We should do the same here and the other line in this utility 
> class.

Good spot, thanks.
Sadly I'm currently working on other code in which Path.of() is not permitted, 
so I won't naturally spot these things.

> test/jdk/jdk/nio/zipfs/Utils.java line 78:
> 
>> 76:  *
>> 77:  * @param name the file name of the jar file to create in the 
>> working directory.
>> 78:  * @param entries a map of relative file name path strings to file 
>> content
> 
> Nit - I think we should replace this description with something like:
> 
>> @param entries a map of JAR file entry names to entry content

Done.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095730823
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095740846
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095740347
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095735890
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095738222

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 13:35:37 GMT, Jaikiran Pai  wrote:

> Did I misread something?

I missed the fact that for a multi-release JAR file, we mark the file system as 
read-only, only if the `multi-release` or the `releaseVersion` environment 
properties have been set. So in this test here, the file system can indeed be 
opened in read-write mode. Sorry about the noise.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095779244

Re: Towards a JSON API for the JDK

[Cay Horstmann]

Il 19/05/25 10:13, Remi Forax ha scritto:

If only there was some deconstruction magic that approximates the JavaScript
code

const doc = { name: "John", age: 30 }
const { name, age } = doc


We already have that, it's called a record :)

Basically, you are advocating for a mapping JsonObject <--> record.

[...]



Cheers,

Cay



Here is a simple JsonObject mapper using java.util.json types.
   https://github.com/forax/json-object-mapper


That's interesting, but I don't think it works as a solution to read generic 
JSON. I have to deal with much JSON that is at best semi-structured. And 
anyway, databinding is is excluded from the scope of the core Java JSON API.

Let me explain in more detail what I was trying say offhandedly. For tree traversal, 
Jackson has isXxx, asXxx, get, path (for "safe" chaining), and JSON Pointer 
support. The core Java JSON library proposes to rely on pattern matching instead. With 
the capabilities of pattern matching today, that is unappealing. Could it get better with 
some of the ideas in 
https://openjdk.org/projects/amber/design-notes/patterns/towards-member-patterns?

Let's assume we have a factory method

var doc = JsonObject.of("name", JsonString.of("John"), "age", 
JsonNumber.of(30));

Could there be a matching deconstructor? It couldn't be overloads like

public inverse JsonObject of(String key1, JsonValue value1, String key2, 
JsonValue value2)

The deconstructor could not know which entries to pick and in what order. But similar to 
the regex example in the design note, one could define a "pattern object" 
holding the keys:

case JsonObject.withKeys("name", "age").of(JsonString.of(String name), 
JsonNumber.of(long age)) ->

Maybe even:

case JsonObject.withKeys("name", "age").fromUntyped(String name, Long age) ->

With nested objects:

case JsonObject.withKeys("user").of(
JsonObject.withKeys("name", "age").fromUntyped(String name, Long age)) -> { 
/* use name, age */ }
default -> /* deal with error */

I made some impromptu design decisions to get this far, as well as assumptions 
how deconstruction would eventually work. Which may well be wrong.

In Jackson, it would be

try {
JsonNode user = nestedDoc.get("user");
String name = user.get("name").asText();
int age = user.get("age").asInt();
/* use name, age */
} catch (...) { /* deal with error */ }

At first I thought the pattern matching version would be worse, but I admit 
that the structural safety is appealing.

Cheers,

Cay



--

Cay S. Horstmann | https://horstmann.com

RFR: 8334015: Add Support for UUID Version 7 (UUIDv7) defined in RFC 9562

[kieran-farrell]

With the recent approval of UUIDv7 (https://datatracker.ietf.org/doc/rfc9562/), 
this PR aims to add a new static method UUID.timestampUUID() which constructs 
and returns a UUID in support of the new time generated UUID version. 

The specification requires embedding the current timestamp in milliseconds into 
the first bits 0–47. The version number in bits 48–51, bits 52–63 are available 
for sub-millisecond precision or for pseudorandom data. The variant is set in 
bits 64–65. The remaining bits 66–127 are free to use for more pseudorandom 
data or to employ a counter based approach for increased time percision 
(https://www.rfc-editor.org/rfc/rfc9562.html#name-uuid-version-7).

The choice of implementation comes down to balancing the sensitivity level of 
being able to distingush UUIDs created below <1ms apart with performance. A 
test simulating a high-concurrency environment with 4 threads generating 1 
UUIDv7 values in parallel to measure the collision rate of each implementation 
(the amount of times the time based portion of the UUID was not unique and 
entries could not distinguished by time) yeilded the following results for each 
implemtation:


- random-byte-only - 99.8%
- higher-precision - 3.5%
- counter-based - 0%


Performance tests show a decrease in performance as expected with the counter 
based implementation due to the introduction of synchronization:

- random-byte-only   143.487 ± 10.932  ns/op
- higher-precision  149.651 ±  8.438 ns/op
- counter-based 245.036 ±  2.943  ns/op

The best balance here might be to employ a higher-precision implementation as 
the large increase in time sensitivity comes at a very slight performance cost.

-

Commit messages:
 - update to add ns percision
 - 3 variantions
 - UUID src and test

Changes: https://git.openjdk.org/jdk/pull/25303/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25303&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8334015
  Stats: 108 lines in 2 files changed: 104 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/25303.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25303/head:pull/25303

PR: https://git.openjdk.org/jdk/pull/25303

ClassLoader.definePackage() throwing IllegalArgumentException

[Robert Stupp]

Hi,

I'd like to follow up on https://bugs.openjdk.org/browse/JDK-8350547.

TL;DR java.lang.ClassLoader.definePackage() (the one taking 8 arguments) 
clearly defines that a IllegalArgumentException is thrown, "if a package 
of the given name is already defined by this class loader".


However, it seems that it's a common oversight that an IAE can be 
thrown. Examples are the CDI reference implementation ("Weld") and 
Quarkus. In other words: "naively" calling 'definePackage' without a 
retry on IAE leads to issues when defining classes in custom class 
loaders in parallel.


The CL.definePackage() API is there since Java 1.2, and I admit that my 
initial approach to change the currently observed behavior isn't going 
to fly.


I thought about adding more information to the message of the IAE thrown 
in 'definePackage()', but even this "innocent" change could break the 
observed behavior / existing code.


One option could be to add a new function 'definePackageIfNotExists()` 
to j.l.ClassLoader, which works like the existing 'definePackage()' but 
returns the existing package, if it already exists and is compatible 
with the given arguments. The downside is that it could still throw, if 
the arguments are incompatible (e.g. a different "implTitle") - so it 
might just be a less likely, but still possible way of unexpectedly 
hitting an IAE.


With 'definePackageIfNotExists()` it might be possible to deprecate 
'definePackage()', but that deprecation largely depends on the actual 
usage of it in the wild, which I do not know. I'm not sure whether such 
a deprecation can fly. And the next question would be whether it can be 
deprecated for removal ; and if yes, when could it be removed.


Actually, even a new 'definePackageIfNotExists()` could break existing 
code, in case an implementation overrides the existing 'definePackage' 
but not 'definePackageIfNotExists()`.


Another question to which I don't know the answer is whether the 7 
package attribute parameters specification/implementation 
title/version/vendor + seal-base do fit "all today's wishes & needs". 
Or: is there demand for a "better" or "more flexible" way to define a 
class/package? Or: can that API be redefined in a different way tailored 
for today's needs and wishes? And are there any that would justify such 
an effort?


Overall, I'm a bit lost on what the "best" way would be for this. Maybe 
the answer is that there is no better way yet, which is also fine. Maybe 
the only "fix" is to enhance the Javadocs and prominently highlight the 
behavior and add a "reference code snippet" how 'definePackage()' should 
be used and why.


Robert

--
Robert Stupp
@snazy

Re: RFR: 8354556: Expand value-based class warnings to java.lang.ref API [v18]

[Vicente Romero]

> This PR is defining a new internal annotation, 
> `@jdk.internal.RequiresIdentity`, with target types PARAMETER and 
> TYPE_PARAMETER. The @RequiresIdentity annotation expresses the expectation 
> that an argument to a given method or constructor parameter will be an object 
> with a unique identity, not an instance of a value-based class; or that the 
> type argument to a given type parameter will not be a value-based class type.
> 
> For more details please refer to the complete description in the 
> corresponding JIRA entry [1]
> 
> TIA
> 
> [1] https://bugs.openjdk.org/browse/JDK-8354556

Vicente Romero has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 39 commits:

 - Merge branch 'master' into JDK-8354556
 - review comments
 - addressing review comments
 - addressing review comments
 - update warning message
 - Merge branch 'master' into JDK-8354556
 - Update src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java
   
   Co-authored-by: Chen Liang 
 - additional changes from Archie
 - removing dead code
 - integrating code from Archie
 - ... and 29 more: https://git.openjdk.org/jdk/compare/265d6301...904bd4cd

-

Changes: https://git.openjdk.org/jdk/pull/24746/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24746&range=17
  Stats: 647 lines in 32 files changed: 562 ins; 37 del; 48 mod
  Patch: https://git.openjdk.org/jdk/pull/24746.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/24746/head:pull/24746

PR: https://git.openjdk.org/jdk/pull/24746

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v8]

[David Beaumont]

> Adding read-only support to ZipFileSystem.
> 
> The new `accessMode` environment property allows for readOnly and readWrite 
> values, and ensures that the requested mode is consistent with what's 
> returned.
> 
> This involved a little refactoring to ensure that "read only" state was set 
> initially and only unset at the end of initialization if appropriate.
> 
> By making 2 methods return values (rather than silently set non-final fields 
> as a side effect) it's now clear in what order fields are initialized and 
> which are final (sadly there are still non-final fields, but only a split of 
> this class into two types can fix that, since determining multi-jar support 
> requires reading the file system).

David Beaumont has updated the pull request incrementally with one additional 
commit since the last revision:

  Changes based on feedback

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/25178/files
  - new: https://git.openjdk.org/jdk/pull/25178/files/46b19106..3b3fe2d7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=07
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=06-07

  Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/25178.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25178/head:pull/25178

PR: https://git.openjdk.org/jdk/pull/25178

Re: Towards a JSON API for the JDK

[forax]

- Original Message -
> From: "cay horstmann" 
> To: "Remi Forax" 
> Cc: "core-libs-dev" 
> Sent: Monday, May 19, 2025 4:12:55 PM
> Subject: Re: Towards a JSON API for the JDK

> Il 19/05/25 10:13, Remi Forax ha scritto:
>>> If only there was some deconstruction magic that approximates the JavaScript
>>> code
>>>
>>> const doc = { name: "John", age: 30 }
>>> const { name, age } = doc
>> 
>> We already have that, it's called a record :)
>> 
>> Basically, you are advocating for a mapping JsonObject <--> record.
>> 
>> [...]
>> 
>>>
>>> Cheers,
>>>
>>> Cay
>>>
>> 
>> Here is a simple JsonObject mapper using java.util.json types.
>>https://github.com/forax/json-object-mapper
> 
> That's interesting, but I don't think it works as a solution to read generic
> JSON. I have to deal with much JSON that is at best semi-structured. And
> anyway, databinding is is excluded from the scope of the core Java JSON API.

If the mapping is driven by the record definition then mapping and matching are 
mostly equivalent,
mapping throws exceptions when something goes wrong while matching returns 
something saying no-match.

Here is an example,

record Person(String name, int age) {}

var recordMapper = RecordMapper.of(MethodHandles.lookup());

JsonObject json = ...

if (recordMapper.match(json, Person.class) instanceof Person(String name, 
int age)) {
  // can use name and age here
}

see 
https://github.com/forax/json-object-mapper/blob/master/src/test/java/json/RecordMapperTest.java#L40

[...]

> 
> At first I thought the pattern matching version would be worse, but I admit 
> that
> the structural safety is appealing.

I agree.

> 
> Cheers,
> 
> Cay

Rémi

Re: RFR: 8356995: Provide default methods min(T, T) and max(T, T) in Comparator interface

[Archie Cobbs]

On Mon, 19 May 2025 07:25:17 GMT, Tagir F. Valeev  wrote:

> I'm not sure whether we should specify exactly the behavior in case if the 
> comparator returns 0. I feel that it could be a useful invariant that 
> `Comparator.min(a, b)` and `Comparator.max(a, b)` always return different 
> argument, partitioning the set of {a, b} objects (even if they are equal). 
> But I'm open to suggestions here.

IMHO it makes sense. It's the min/max analog to a stable sort.

-

PR Comment: https://git.openjdk.org/jdk/pull/25297#issuecomment-2891353170

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v5]

[David Beaumont]

> Adding read-only support to ZipFileSystem.
> 
> The new `accessMode` environment property allows for readOnly and readWrite 
> values, and ensures that the requested mode is consistent with what's 
> returned.
> 
> This involved a little refactoring to ensure that "read only" state was set 
> initially and only unset at the end of initialization if appropriate.
> 
> By making 2 methods return values (rather than silently set non-final fields 
> as a side effect) it's now clear in what order fields are initialized and 
> which are final (sadly there are still non-final fields, but only a split of 
> this class into two types can fix that, since determining multi-jar support 
> requires reading the file system).

David Beaumont has updated the pull request incrementally with one additional 
commit since the last revision:

  Changes based on review feedback.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/25178/files
  - new: https://git.openjdk.org/jdk/pull/25178/files/2326999b..a82e4019

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=03-04

  Stats: 65 lines in 4 files changed: 35 ins; 7 del; 23 mod
  Patch: https://git.openjdk.org/jdk/pull/25178.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25178/head:pull/25178

PR: https://git.openjdk.org/jdk/pull/25178

Re: RFR: 8356891: Some code simplifications for basic BigIntegers' bit operations [v8]

[Raffaello Giulietti]

On Fri, 16 May 2025 11:10:12 GMT, fabioromano1  wrote:

>> Some changes in `Biginteger`s' bit operations that increase the code 
>> readability and slightly optimize the execution time.
>
> fabioromano1 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   An optimization

src/java.base/share/classes/java/math/BigInteger.java line 5034:

> 5032:  */
> 5033: private byte[] magSerializedForm() {
> 5034: byte[] result = new byte[(magBitLength() + 7) >>> 3];

I think there's a risk of overflow, so consider
Suggestion:

byte[] result = new byte[(magBitLength() - 1 >>> 3) + 1];

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25166#discussion_r2095920641

Re: RFR: 8356891: Some code simplifications for basic BigIntegers' bit operations [v8]

[Raffaello Giulietti]

On Mon, 19 May 2025 15:02:57 GMT, Raffaello Giulietti  
wrote:

>> fabioromano1 has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   An optimization
>
> src/java.base/share/classes/java/math/BigInteger.java line 5034:
> 
>> 5032:  */
>> 5033: private byte[] magSerializedForm() {
>> 5034: byte[] result = new byte[(magBitLength() + 7) >>> 3];
> 
> I think there's a risk of overflow, so consider
> Suggestion:
> 
> byte[] result = new byte[(magBitLength() - 1 >>> 3) + 1];

Ah no, there's a unsigned shift `>>>`.
Disregard

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25166#discussion_r2095922459

Re: Towards a JSON API for the JDK

[Remi Forax]

Okay, 
i've taken a look to the design and this is not pretty. 

The main issue is that the javadoc claims that 
"Both JsonValue instances and their underlying values are immutable." 
but at the same time any subtypes of JsonValue is non-sealed so anyone can 
implement let say JsonString and adds it's own mutable implementation. 

Because the hierarchy is non sealed, it also means that it is easy to create 
JsonValue that are invalid, 
for example 

var funJsonNumber = new JsonNumber() { 
public Number toNumber () { 
return Double . NaN ; 
} 
public BigDecimal toBigDecimal () { 
throw new UnsupportedOperationException(); 
} 
public String toString () { 
return "NaN" ; 
} 
}; 

var json = Json.fromUntyped( List . of ( funJsonNumber )); 

For me, the Json hierarchy should be implemented with true ADTs, with all 
subtypes of JsonValue being records. 

regards, 
Rémi 

> From: "Remi Forax" 
> To: "Brian Goetz" 
> Cc: "Paul Sandoz" , "core-libs-dev"
> 
> Sent: Friday, May 16, 2025 8:42:47 PM
> Subject: Re: Towards a JSON API for the JDK

>> From: "Brian Goetz" 
>> To: "Remi Forax" 
>> Cc: "Paul Sandoz" , "core-libs-dev"
>> 
>> Sent: Friday, May 16, 2025 7:46:09 PM
>> Subject: Re: Towards a JSON API for the JDK

>> If you read the implementation, you'll see that significant laziness is 
>> indeed
>> possible for JsonObject and JsonArray, even while doing eager validation. (Of
>> course, one can shift the balance to achieve various other tradeoffs.)
> Reading the implementation is on my TODO list :)

> Rémi

>> On 5/16/2025 10:35 AM, [ mailto:fo...@univ-mlv.fr | fo...@univ-mlv.fr ] 
>> wrote:

>>> - Original Message -

 From: "Brian Goetz" [ mailto:brian.go...@oracle.com | 
  ]
 To: "Remi Forax" [ mailto:fo...@univ-mlv.fr |  ] , "Paul
 Sandoz" [ mailto:paul.san...@oracle.com |  ] Cc:
 "core-libs-dev" [ mailto:core-libs-dev@openjdk.org |
  ] Sent: Friday, May 16, 2025 2:53:18 PM
 Subject: Re: Towards a JSON API for the JDK

 On 5/15/2025 5:27 PM, Remi Forax wrote:

> It's not clear to me why JsonArray (for example) has to be an interface 
> instead
> of a record ?

 Oh, you know the answer to this.  A record limits us to a single
 implementation with a rigid representation.  Behind an interface, we can
 hide lazy parsing and inflation, wrapping other representations to
 reduce copies, etc.

>>> First, let me refine the question.
>>> There are only 4 kinds of JSON values that benefit from having different
>>> representations, object, array, string and number.
>>> For object and array, both takes an interface as parameter (Map or List) so
>>> JsonArray and JSonObject do not need to be themselves interfaces.

>>> So the only values where it may be worth to be modeled using an interface 
>>> are
>>> JsonString and JsonNumber, because as you said, you can do "lazy" parsing.

>>> But delaying the parsing of the content has the side effect that even if
>>> Json.parse() did not throw an exception, it does not mean that the JSON 
>>> text is
>>> valid, an exception may be thrown later.

>>> Now, for me, this library is more about being simple and safe than fast.
>>> If you agree with that, delaying the parsing is not a good idea, thus JSON
>>> values should be modeled using records and not interfaces.

>>> regards,
>>> Rémi

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v9]

[David Beaumont]

> Adding read-only support to ZipFileSystem.
> 
> The new `accessMode` environment property allows for readOnly and readWrite 
> values, and ensures that the requested mode is consistent with what's 
> returned.
> 
> This involved a little refactoring to ensure that "read only" state was set 
> initially and only unset at the end of initialization if appropriate.
> 
> By making 2 methods return values (rather than silently set non-final fields 
> as a side effect) it's now clear in what order fields are initialized and 
> which are final (sadly there are still non-final fields, but only a split of 
> this class into two types can fix that, since determining multi-jar support 
> requires reading the file system).

David Beaumont has updated the pull request incrementally with one additional 
commit since the last revision:

  Don't use JUnit utils in TestNg.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/25178/files
  - new: https://git.openjdk.org/jdk/pull/25178/files/3b3fe2d7..e5725599

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=08
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25178&range=07-08

  Stats: 6 lines in 1 file changed: 1 ins; 1 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/25178.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25178/head:pull/25178

PR: https://git.openjdk.org/jdk/pull/25178

Re: RFR: 8356891: Some code simplifications for basic BigIntegers' bit operations [v8]

[fabioromano1]

On Mon, 19 May 2025 15:03:51 GMT, Raffaello Giulietti  
wrote:

>> src/java.base/share/classes/java/math/BigInteger.java line 5034:
>> 
>>> 5032:  */
>>> 5033: private byte[] magSerializedForm() {
>>> 5034: byte[] result = new byte[(magBitLength() + 7) >>> 3];
>> 
>> I think there's a risk of overflow, so consider
>> Suggestion:
>> 
>> byte[] result = new byte[(magBitLength() - 1 >>> 3) + 1];
>
> Ah no, there's a unsigned shift `>>>`.
> Disregard

Indeed, I had thought about this possibility too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25166#discussion_r2095957646

Re: RFR: 8355954: File.delete removes read-only files (win) [v4]

[Brian Burkhalter]

On Fri, 16 May 2025 17:22:14 GMT, Brian Burkhalter  wrote:

>> I guess collapsing the tests made them more ambiguous. Will fix.
>
> Split the test method in half for Unix and Windows in 
> [fa2273e](https://github.com/openjdk/jdk/pull/24977/commits/fa2273eded040a22c1e32ba8870571d3a7daf427).

> The behavior and system property is Windows specific and might make things 
> simpler to make it a Windows only test.

I think I might have misunderstood this comment. Was the intent to make this 
test use `@requires os.family == "windows"`?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24977#discussion_r2095962959

Re: RFR: 8357179: Deprecate VFORK launch mechanism from Process implementation (linux)

[Roger Riggs]

On Sat, 17 May 2025 06:49:37 GMT, Thomas Stuefe  wrote:

> For the ratio behind this please see the companion CSR: 
> https://bugs.openjdk.org/browse/JDK-8357180.
> 
> We plan to deprecate this in JDK 25 and to remove it in JDK 26.
> 
> This patch just writes a deprecation message to stderr:
> 
> 
> 08:46:38 thomas@starfish java -Djdk.lang.Process.launchMechanism=VFORK 
> SimpleSpawn ls
> VFORK MODE DEPRECATED
> The VFORK launch mechanism has been deprecated for being dangerous.
> It will be removed in a future java version. Either remove the
> jdk.lang.Process.launchMechanism property (preferred) or use FORK mode
> instead (-Djdk.lang.Process.launchMechanism=FORK).
> ...

lgtm;  thanks

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25282#pullrequestreview-2851284733

Re: RFR: 8356891: Some code simplifications for basic BigIntegers' bit operations [v6]

[Raffaello Giulietti]

On Fri, 16 May 2025 13:30:01 GMT, fabioromano1  wrote:

>> src/java.base/share/classes/java/math/BigInteger.java line 3860:
>> 
>>> 3858: && Integer.lowestOneBit(mag[0]) == mag[0]
>>> 3859: && numberOfTrailingZeroInts() == mag.length - 1
>>> 3860: ? magBitLength() - 1 : magBitLength();
>> 
>> Suggestion:
>> 
>> int[] mag = this.mag;
>> return magBitLength() 
>> - (signum < 0
>>// Check if magnitude is a power of two
>>&& Integer.lowestOneBit(mag[0]) == mag[0]
>>&& numberOfTrailingZeroInts() == mag.length - 1
>>? 1 : 0);
>> 
>> 
>> In this way, codeSize will drop from 52 to 45
>
> What is the usefulness of doing `int[] mag = this.mag;` at line 3856, since 
> `bitLength()` does not access to `static` fields and `BigInteger` is not 
> threadsafe, and therefore no race conditions occur?

I think the suggestion is fine.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25166#discussion_r2096038516

Re: RFR: 8356891: Some code simplifications for basic BigIntegers' bit operations [v9]

[fabioromano1]

> Some changes in `Biginteger`s' bit operations that increase the code 
> readability and slightly optimize the execution time.

fabioromano1 has updated the pull request incrementally with one additional 
commit since the last revision:

  Removed numberOfTrailingZeros() as a duplicate of the cached getLowestSetBit()

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/25166/files
  - new: https://git.openjdk.org/jdk/pull/25166/files/b151b7e3..b74fefa5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25166&range=08
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25166&range=07-08

  Stats: 18 lines in 1 file changed: 5 ins; 11 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/25166.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25166/head:pull/25166

PR: https://git.openjdk.org/jdk/pull/25166

Re: RFR: 8355954: File.delete removes read-only files (win) [v4]

[Alan Bateman]

On Mon, 19 May 2025 15:20:25 GMT, Brian Burkhalter  wrote:

>> Split the test method in half for Unix and Windows in 
>> [fa2273e](https://github.com/openjdk/jdk/pull/24977/commits/fa2273eded040a22c1e32ba8870571d3a7daf427).
>
>> The behavior and system property is Windows specific and might make things 
>> simpler to make it a Windows only test.
> 
> I think I might have misunderstood this comment. Was the intent to make this 
> test use `@requires os.family == "windows"`?

I think that would be simpler and means the `@EnabledOnOs` usages would go 
away, but it's up to you.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24977#discussion_r2096050597

Re: RFR: 8357179: Deprecate VFORK launch mechanism from Process implementation (linux)

[Alan Bateman]

On Sat, 17 May 2025 06:49:37 GMT, Thomas Stuefe  wrote:

> For the ratio behind this please see the companion CSR: 
> https://bugs.openjdk.org/browse/JDK-8357180.
> 
> We plan to deprecate this in JDK 25 and to remove it in JDK 26.
> 
> This patch just writes a deprecation message to stderr:
> 
> 
> 08:46:38 thomas@starfish java -Djdk.lang.Process.launchMechanism=VFORK 
> SimpleSpawn ls
> VFORK MODE DEPRECATED
> The VFORK launch mechanism has been deprecated for being dangerous.
> It will be removed in a future java version. Either remove the
> jdk.lang.Process.launchMechanism property (preferred) or use FORK mode
> instead (-Djdk.lang.Process.launchMechanism=FORK).
> ...

src/java.base/unix/classes/java/lang/ProcessImpl.java line 111:

> 109:"It will be removed in a 
> future java version. Either remove the\n" +
> 110:
> "jdk.lang.Process.launchMechanism property (preferred) or use FORK mode\n" +
> 111:"instead 
> (-Djdk.lang.Process.launchMechanism=FORK).");

This is okay although I could imagine someone wanting to replace this with a 
text block.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25282#discussion_r2096052329

Re: RFR: 8354556: Expand value-based class warnings to java.lang.ref API [v18]

[Jan Lahoda]

On Mon, 19 May 2025 14:35:40 GMT, Vicente Romero  wrote:

>> This PR is defining a new internal annotation, 
>> `@jdk.internal.RequiresIdentity`, with target types PARAMETER and 
>> TYPE_PARAMETER. The @RequiresIdentity annotation expresses the expectation 
>> that an argument to a given method or constructor parameter will be an 
>> object with a unique identity, not an instance of a value-based class; or 
>> that the type argument to a given type parameter will not be a value-based 
>> class type.
>> 
>> For more details please refer to the complete description in the 
>> corresponding JIRA entry [1]
>> 
>> TIA
>> 
>> [1] https://bugs.openjdk.org/browse/JDK-8354556
>
> Vicente Romero has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 39 commits:
> 
>  - Merge branch 'master' into JDK-8354556
>  - review comments
>  - addressing review comments
>  - addressing review comments
>  - update warning message
>  - Merge branch 'master' into JDK-8354556
>  - Update src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java
>
>Co-authored-by: Chen Liang 
>  - additional changes from Archie
>  - removing dead code
>  - integrating code from Archie
>  - ... and 29 more: https://git.openjdk.org/jdk/compare/265d6301...904bd4cd

javac changes look good to me.

-

Marked as reviewed by jlahoda (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24746#pullrequestreview-2851310025

RFR: 8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base

[Nizar Benalla]

Please review this patch to fix some `javadoc` bugs in `java.base`.
Certain `@link` tags used to refer to private fields instead of public APIs.

A couple of `@see` tags in the [serialization 
page](https://download.java.net/java/early_access/jdk25/docs/api/serialized-form.html#java.lang.invoke.MethodType)
 referred to private methods, I updated the javadoc in a way to not change the 
way it is displayed to users but also remove `@link` tags to non-included types.

TIA

-

Commit messages:
 - respond to feedback
 - fix incorrect `@link` tags

Changes: https://git.openjdk.org/jdk/pull/25287/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25287&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8356632
  Stats: 14 lines in 6 files changed: 0 ins; 0 del; 14 mod
  Patch: https://git.openjdk.org/jdk/pull/25287.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25287/head:pull/25287

PR: https://git.openjdk.org/jdk/pull/25287

Re: RFR: 8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base

[ExE Boss]

On Sun, 18 May 2025 02:17:46 GMT, Chen Liang  wrote:

>> Please review this patch to fix some `javadoc` bugs in `java.base`.
>> Certain `@link` tags used to refer to private fields instead of public APIs.
>> 
>> A couple of `@see` tags in the [serialization 
>> page](https://download.java.net/java/early_access/jdk25/docs/api/serialized-form.html#java.lang.invoke.MethodType)
>>  referred to private methods, I updated the javadoc in a way to not change 
>> the way it is displayed to users but also remove `@link` tags to 
>> non-included types.
>> 
>> TIA
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4893:
> 
>> 4891:  * @return a constant method handle of the given type, which 
>> returns a default value of the given return type
>> 4892:  * @throws NullPointerException if the argument is null
>> 4893:  * @see primitiveZero(Wrapper)
> 
> Just `#zero(Class)` should be sufficient. This is probably changed by 
> intellij during my original work to consolidate zero and constant.

From :
Suggestion:

 * @see MethodHandles#zero

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25287#discussion_r2094592478

Re: RFR: 8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base

[Nizar Benalla]

On Sun, 18 May 2025 02:18:24 GMT, Chen Liang  wrote:

>> Please review this patch to fix some `javadoc` bugs in `java.base`.
>> Certain `@link` tags used to refer to private fields instead of public APIs.
>> 
>> A couple of `@see` tags in the [serialization 
>> page](https://download.java.net/java/early_access/jdk25/docs/api/serialized-form.html#java.lang.invoke.MethodType)
>>  referred to private methods, I updated the javadoc in a way to not change 
>> the way it is displayed to users but also remove `@link` tags to 
>> non-included types.
>> 
>> TIA
>
> src/java.base/share/classes/java/lang/invoke/MethodType.java line 1341:
> 
>> 1339:  * @throws ClassNotFoundException if one of the component classes 
>> cannot be resolved
>> 1340:  * @see MethodType.readResolve()
>> 1341:  * @see MethodType.writeObject(ObjectOutputStream s)
> 
> This is already not generated by the javadoc as this method is private. Why 
> is this check against a private method?

They are documented in [this 
page](https://download.java.net/java/early_access/jdk25/docs/api/serialized-form.html#java.lang.invoke.MethodType)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25287#discussion_r2095501605

Re: RFR: 8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base

[Chen Liang]

On Sat, 17 May 2025 19:42:39 GMT, Nizar Benalla  wrote:

> Please review this patch to fix some `javadoc` bugs in `java.base`.
> Certain `@link` tags used to refer to private fields instead of public APIs.
> 
> A couple of `@see` tags in the [serialization 
> page](https://download.java.net/java/early_access/jdk25/docs/api/serialized-form.html#java.lang.invoke.MethodType)
>  referred to private methods, I updated the javadoc in a way to not change 
> the way it is displayed to users but also remove `@link` tags to non-included 
> types.
> 
> TIA

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4893:

> 4891:  * @return a constant method handle of the given type, which 
> returns a default value of the given return type
> 4892:  * @throws NullPointerException if the argument is null
> 4893:  * @see primitiveZero(Wrapper)

Just `#zero(Class)` should be sufficient. This is probably changed by intellij 
during my original work to consolidate zero and constant.

src/java.base/share/classes/java/lang/invoke/MethodType.java line 1341:

> 1339:  * @throws ClassNotFoundException if one of the component classes 
> cannot be resolved
> 1340:  * @see MethodType.readResolve()
> 1341:  * @see MethodType.writeObject(ObjectOutputStream s)

This is already not generated by the javadoc as this method is private. Why is 
this check against a private method?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25287#discussion_r2094316583
PR Review Comment: https://git.openjdk.org/jdk/pull/25287#discussion_r2094316628

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[Jaikiran Pai]

On Mon, 19 May 2025 13:31:04 GMT, David Beaumont  wrote:

>> test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 208:
>> 
>>> 206: // Multi-release JARs, when opened with a specified version 
>>> are inherently read-only.
>>> 207: Path multiReleaseJar = createMultiReleaseJar();
>>> 208: try (FileSystem fs = 
>>> FileSystems.newFileSystem(multiReleaseJar, Map.of("accessMode", 
>>> "readWrite"))) {
>> 
>> I wonder if the ZIP filesystem implementation should throw an exception in 
>> this case. In the case where the application code has explicitly stated 
>> `accessMode=readWrite`, if the underlying file `Path` isn't writable, then 
>> we currently throw an `IOException`. I think we should specify and implement 
>> the same for the case where `accessMode=readWrite` and the file is a 
>> multi-release JAR file.
>
> It does throw if accessMode=readWrite and the result is not writable for any 
> reason.
> 
> 
> this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() || 
> !Files.isWritable(zfpath);
> if (readOnly && accessMode == ZipAccessMode.READ_WRITE) {

Actually looking at the current proposed implementation in ZipFileSystem where 
we have:



this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() || 
!Files.isWritable(zfpath);
if (readOnly && accessMode == ZipAccessMode.READ_WRITE) {
String reason = multiReleaseVersion.isPresent()
? "the multi-release JAR file is not writable"
: "the ZIP file is not writable";
throw new IOException(reason);
}


I'm surprised this test currently passes. Shouldn't this line in the test lead 
to an `IOException`:

FileSystems.newFileSystem(multiReleaseJar, Map.of("accessMode", "readWrite")))


Did I misread something?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095732163

Withdrawn: 8349545: ClassLoader.definePackage() throws IllegalArgumentException if package already defined

[duke]

On Sun, 23 Feb 2025 11:32:41 GMT, Robert Stupp  wrote:

> Concurent calls to `ClassLoader.definePackage()` can yield 
> `IllegalArgumentException`s if the package is already defined. Some built-in 
> class loaders, like `URLClassLoader`, already handle this case, but custom 
> class loaders (would) have to handle this case.
> 
> This change updates the logic of `CL.definePackage` to not throw an IAE if 
> the "redefined" package is equal to the already defined one.
> 
> Tests added in `jdk/java/lang/Package/PackageDefineTest.java` (+ pulling up 
> the `TestClassLoader` from `PackageDefineTest`).

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.org/jdk/pull/23737

Integrated: 8357106: Add missing classpath exception copyright headers

[Sorna Sarathi N]

On Fri, 16 May 2025 07:04:04 GMT, Sorna Sarathi N  wrote:

> This PR adds missing classpath exception

This pull request has now been integrated.

Changeset: afcaf840
Author:Sorna Sarathi N 
Committer: Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/afcaf84022f165d66068c16460b7666f48e84773
Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod

8357106: Add missing classpath exception copyright headers

Reviewed-by: jpai

-

PR: https://git.openjdk.org/jdk/pull/25261

Re: RFR: 8357178: Simplify Class::componentType [v2]

[Joe Darcy]

On Sun, 18 May 2025 21:15:21 GMT, Chen Liang  wrote:

> > Did you consider calling getComponentType(), which already returns the 
> > field?
> 
> I don't think we want an extra indirection here - The logic here is quite 
> simple. Counterargument could be that `MethodType::descriptorString` calls 
> `toMethodDescriptorString`, except these methods involve more complex caching 
> and is not simple like this field access.

My mental model anyway is that this trivial inline would be something the VM 
would do readily.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25280#discussion_r2096089013

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v6]

[David Beaumont]

On Mon, 19 May 2025 12:54:37 GMT, Jaikiran Pai  wrote:

>> David Beaumont has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fixed test.
>
> test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 224:
> 
>> 222: // Underlying file is read-only.
>> 223: Path readOnlyZip = Utils.createJarFile("read_only.zip", 
>> Map.of("file.txt", "Hello World"));
>> 224: readOnlyZip.toFile().setReadOnly();
> 
> `java.io.File.setReadOnly()` specifies:
> 
>> On some platforms it may be possible to start the
>> Java virtual machine with special privileges that allow it to modify
>> files that are marked read-only. Whether or not a read-only file or
>> directory may be deleted depends upon the underlying system.
> 
> So I think we should run the subsequent asserts in this test after first 
> checking if the file was set to read-only. If it isn't then we should skip 
> the test. Something like:
> 
> 
> final boolean marked  = readOnlyZip.toFile().setReadOnly();
> Assumptions.assumeTrue(marked, "skipping test since " + readOnlyZip + " 
> couldn't be marked read-only");
> assertThrows(IOException.class,
> () -> FileSystems.newFileSystem(readOnlyZip, 
> Map.of("accessMode", "readWrite")));

Done. Thanks for introducing me to the Assumptions class :)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095780258

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v9]

[Alan Bateman]

On Mon, 19 May 2025 15:18:12 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Don't use JUnit utils in TestNg.

src/jdk.zipfs/share/classes/module-info.java line 264:

> 262:  *   {@linkplain Runtime.Version Java SE Platform version 
> number},
> 263:  *   an {@code IllegalArgumentException} will be thrown when 
> the Zip
> 264:  *   filesystem is created.

I think this needs to "when creating the ZIP file system".

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2096125703

Re: RFR: 8350880: (zipfs) Add support for read-only zip file systems [v9]

[Alan Bateman]

On Mon, 19 May 2025 15:18:12 GMT, David Beaumont  wrote:

>> Adding read-only support to ZipFileSystem.
>> 
>> The new `accessMode` environment property allows for readOnly and readWrite 
>> values, and ensures that the requested mode is consistent with what's 
>> returned.
>> 
>> This involved a little refactoring to ensure that "read only" state was set 
>> initially and only unset at the end of initialization if appropriate.
>> 
>> By making 2 methods return values (rather than silently set non-final fields 
>> as a side effect) it's now clear in what order fields are initialized and 
>> which are final (sadly there are still non-final fields, but only a split of 
>> this class into two types can fix that, since determining multi-jar support 
>> requires reading the file system).
>
> David Beaumont has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Don't use JUnit utils in TestNg.

src/jdk.zipfs/share/classes/module-info.java line 288:

> 286:  *   isReadOnly()} will always return {@code true}. Creating 
> a
> 287:  *   read-only file system requires the underlying 
> ZIP file to
> 288:  *   already exist, and is incompatible with {@code 
> "create"=true}.

I think it would be clearer to drop "and is incompatible with {@code 
"create"=true}". The sentence that follows makes it clear that 
accessMode=readWrite && create=true cause IAE to be thrown.

src/jdk.zipfs/share/classes/module-info.java line 291:

> 289:  *   Specifying {@code create} as {@code true} and {@code 
> accessMode} as
> 290:  *   {@code readOnly} will cause an {@code 
> IllegalArgumentException}
> 291:  *   to be thrown when the ZIP filesystem is created.

"created" sounds past tense, so making it "when creating the ZIP file system" 
would work better here. Same comment on the text further down.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2096133967
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2096134802

Integrated: 8351230: Collections.synchronizedList returns a list that is not thread-safe

[Stuart Marks]

On Thu, 1 May 2025 19:05:50 GMT, Stuart Marks  wrote:

> Collections.synchronizedList() returns a List implementation that doesn't do 
> proper locking. This PR does the following on the synchronized wrapper:
> 
> - overrides and adds locking to SequencedCollection methods;
> - performs instance management of reversed synchronized views;
> - adds test for race conditions and functional tests of synchronized wrappers.

This pull request has now been integrated.

Changeset: 6818dcc0
Author:Stuart Marks 
URL:   
https://git.openjdk.org/jdk/commit/6818dcc08ed85e220c5206fda5c991b886e35334
Stats: 260 lines in 3 files changed: 247 ins; 9 del; 4 mod

8351230: Collections.synchronizedList returns a list that is not thread-safe

Reviewed-by: jpai

-

PR: https://git.openjdk.org/jdk/pull/24990

Re: RFR: 8357179: Deprecate VFORK launch mechanism from Process implementation (linux) [v2]

[Thomas Stuefe]

> For the ratio behind this please see the companion CSR: 
> https://bugs.openjdk.org/browse/JDK-8357180.
> 
> We plan to deprecate this in JDK 25 and to remove it in JDK 26.
> 
> This patch just writes a deprecation message to stderr:
> 
> 
> 08:46:38 thomas@starfish java -Djdk.lang.Process.launchMechanism=VFORK 
> SimpleSpawn ls
> VFORK MODE DEPRECATED
> The VFORK launch mechanism has been deprecated for being dangerous.
> It will be removed in a future java version. Either remove the
> jdk.lang.Process.launchMechanism property (preferred) or use FORK mode
> instead (-Djdk.lang.Process.launchMechanism=FORK).
> ...

Thomas Stuefe has updated the pull request incrementally with one additional 
commit since the last revision:

  use string block

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/25282/files
  - new: https://git.openjdk.org/jdk/pull/25282/files/8bd4a36e..ec3197f9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=25282&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25282&range=00-01

  Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/25282.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25282/head:pull/25282

PR: https://git.openjdk.org/jdk/pull/25282

Re: RFR: 8357179: Deprecate VFORK launch mechanism from Process implementation (linux) [v2]

[Thomas Stuefe]

On Mon, 19 May 2025 15:58:17 GMT, Alan Bateman  wrote:

>> Thomas Stuefe has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   use string block
>
> src/java.base/unix/classes/java/lang/ProcessImpl.java line 111:
> 
>> 109:"It will be removed in a 
>> future java version. Either remove the\n" +
>> 110:
>> "jdk.lang.Process.launchMechanism property (preferred) or use FORK mode\n" +
>> 111:"instead 
>> (-Djdk.lang.Process.launchMechanism=FORK).");
> 
> This is okay although I could imagine someone wanting to replace this with a 
> text block.

okay, changed to string block

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25282#discussion_r2096149401

Re: RFR: 8356995: Provide default methods min(T, T) and max(T, T) in Comparator interface

[Raffaello Giulietti]

On Mon, 19 May 2025 07:25:17 GMT, Tagir F. Valeev  wrote:

> Implementation of Comparator.min and Comparator.max methods. Preliminary 
> discussion is in this thread:
> https://mail.openjdk.org/pipermail/core-libs-dev/2025-May/145638.html
> The specification is mostly composed of Math.min/max and Collections.min/max 
> specifications. 
> 
> The methods are quite trivial, so I don't think we need more extensive 
> testing (e.g., using different comparators). But if you have ideas of new 
> useful tests, I'll gladly add them.
> 
> I'm not sure whether we should specify exactly the behavior in case if the 
> comparator returns 0. I feel that it could be a useful invariant that 
> `Comparator.min(a, b)` and `Comparator.max(a, b)` always return different 
> argument, partitioning the set of {a, b} objects (even if they are equal). 
> But I'm open to suggestions here.

@amaembo Could you please switch the test class to make use of 
[JUnit](https://junit.org/junit5/)? For new tests we now prefer JUnit over 
TestNG.
Thanks!

-

PR Comment: https://git.openjdk.org/jdk/pull/25297#issuecomment-2891719275

Re: RFR: 8355954: File.delete removes read-only files (win) [v4]

[Brian Burkhalter]

On Mon, 19 May 2025 15:57:14 GMT, Alan Bateman  wrote:

> I think that would be simpler and means the `@EnabledOnOs` usages would go 
> away, but it's up to you.

I agree that is better. So changed in 
[74e1698](https://github.com/openjdk/jdk/pull/24977/commits/74e16983c94d9b29c91b660e8a025388d72aee71).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/24977#discussion_r2096163983

Re: RFR: 8355954: File.delete removes read-only files (win) [v7]

[Brian Burkhalter]

> This change proposes to modify `java.io.File.delete()` so that regular files 
> on Windows will not be deleted by default if their read-only attribute is 
> set. A boolean-valued system compatibility property 
> `jdk.io.File.deleteReadOnly` is defined to reinstate legacy behavior if 
> desired.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8355954: Make test Windows-only

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/24977/files
  - new: https://git.openjdk.org/jdk/pull/24977/files/9fa45418..74e16983

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=24977&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24977&range=05-06

  Stats: 16 lines in 1 file changed: 1 ins; 14 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/24977.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/24977/head:pull/24977

PR: https://git.openjdk.org/jdk/pull/24977

Re: Towards a JSON API for the JDK

[Johannes Döbler]

Developers use JSON libraries like Fastjson, Jackson and Gson primarily 
(my claim) because they can marshal between JSON files and POJOs in one 
line of code with great performance and can easily tweak the mapping 
using annotations or adapter classes.
Being able to read/write a JSON file from/to a generic tree of 
JsonValues might have use-cases, but manually implementing data-binding 
based one tree traversal w/wo pattern matching will produce bloated and 
fragile code (my claim).
So my question is if there are plans to address data-binding in the 
future and how it would relate to the envisioned core API.

Thanks
Johannes



# Towards a JSON API for the JDK

One of the most common requests for the JDK is an API for parsing and generating
JSON. While JSON originated as a text-based serialization format for JSON
objects ("JSON" stands for "JavaScript Object Notation"), because of its simple
and flexible syntax, it eventually found use outside the JavaScript ecosystem as
a general data interchange format, such as framework configuration files and web
service requests/response formats.

While the JDK cannot, and should not, provide libraries for every conceivable
file format or protocol, the JDK philosophy is one of "batteries included",
which is to say we should be able to write basic programs that use common
protocols such as HTTP, without having to appeal to third party libraries.
The Java ecosystem already has plenty of JSON libraries, so inclusion in
the JDK is largely meant to be a convenience, rather than needing to be the "one
true" JSON library to meet the needs of all users. Users with specific needs
are always free to select one of the existing third-party libraries.

## Goals and requirements

Our primary goal is that the library be simple to use for parsing, traversing,
and generating conformant JSON documents. Advanced features, such as data
binding or path-based traversal should be possible to implement as layered
features, but for simplicity are not included in the core API. We adopt a goal
that the performance should be "good enough", but where performance
considerations conflict with simplicity and usability, we will choose in favor
of the latter.

Re: RFR: 8357179: Deprecate VFORK launch mechanism from Process implementation (linux) [v2]

[Roger Riggs]

On Mon, 19 May 2025 16:59:06 GMT, Thomas Stuefe  wrote:

>> For the ratio behind this please see the companion CSR: 
>> https://bugs.openjdk.org/browse/JDK-8357180.
>> 
>> We plan to deprecate this in JDK 25 and to remove it in JDK 26.
>> 
>> This patch just writes a deprecation message to stderr:
>> 
>> 
>> 08:46:38 thomas@starfish java -Djdk.lang.Process.launchMechanism=VFORK 
>> SimpleSpawn ls
>> VFORK MODE DEPRECATED
>> The VFORK launch mechanism has been deprecated for being dangerous.
>> It will be removed in a future java version. Either remove the
>> jdk.lang.Process.launchMechanism property (preferred) or use FORK mode
>> instead (-Djdk.lang.Process.launchMechanism=FORK).
>> ...
>
> Thomas Stuefe has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use string block

Marked as reviewed by rriggs (Reviewer).

Looks good, also reviewed the CSR, it can be finalized.

-

PR Review: https://git.openjdk.org/jdk/pull/25282#pullrequestreview-2851506982
PR Comment: https://git.openjdk.org/jdk/pull/25282#issuecomment-2891742547

Re: RFR: 8356891: Some code simplifications for basic BigIntegers' bit operations [v6]

[fabioromano1]

On Mon, 19 May 2025 15:50:10 GMT, Raffaello Giulietti  
wrote:

>> What is the usefulness of doing `int[] mag = this.mag;` at line 3856, since 
>> `bitLength()` does not access to `static` fields and `BigInteger` is not 
>> threadsafe, and therefore no race conditions occur?
>
> I think the suggestion is fine.

Yes, but perhaps in the current version it's more clear what is the returned 
value according to the condition.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25166#discussion_r2096183726

Re: ClassLoader.definePackage() throwing IllegalArgumentException

[Alan Bateman]

On 19/05/2025 15:34, Robert Stupp wrote:

Hi,

I'd like to follow up on https://bugs.openjdk.org/browse/JDK-8350547.

TL;DR java.lang.ClassLoader.definePackage() (the one taking 8 
arguments) clearly defines that a IllegalArgumentException is thrown, 
"if a package of the given name is already defined by this class loader".


However, it seems that it's a common oversight that an IAE can be 
thrown. Examples are the CDI reference implementation ("Weld") and 
Quarkus. In other words: "naively" calling 'definePackage' without a 
retry on IAE leads to issues when defining classes in custom class 
loaders in parallel.


The CL.definePackage() API is there since Java 1.2, and I admit that 
my initial approach to change the currently observed behavior isn't 
going to fly.


I thought about adding more information to the message of the IAE 
thrown in 'definePackage()', but even this "innocent" change could 
break the observed behavior / existing code.


One option could be to add a new function 'definePackageIfNotExists()` 
to j.l.ClassLoader, which works like the existing 'definePackage()' 
but returns the existing package, if it already exists and is 
compatible with the given arguments. The downside is that it could 
still throw, if the arguments are incompatible (e.g. a different 
"implTitle") - so it might just be a less likely, but still possible 
way of unexpectedly hitting an IAE.


With 'definePackageIfNotExists()` it might be possible to deprecate 
'definePackage()', but that deprecation largely depends on the actual 
usage of it in the wild, which I do not know. I'm not sure whether 
such a deprecation can fly. And the next question would be whether it 
can be deprecated for removal ; and if yes, when could it be removed.


Actually, even a new 'definePackageIfNotExists()` could break existing 
code, in case an implementation overrides the existing 'definePackage' 
but not 'definePackageIfNotExists()`.


Another question to which I don't know the answer is whether the 7 
package attribute parameters specification/implementation 
title/version/vendor + seal-base do fit "all today's wishes & needs". 
Or: is there demand for a "better" or "more flexible" way to define a 
class/package? Or: can that API be redefined in a different way 
tailored for today's needs and wishes? And are there any that would 
justify such an effort?


Overall, I'm a bit lost on what the "best" way would be for this. 
Maybe the answer is that there is no better way yet, which is also 
fine. Maybe the only "fix" is to enhance the Javadocs and prominently 
highlight the behavior and add a "reference code snippet" how 
'definePackage()' should be used and why.


As we discussed already, this API "awkwardness" goes back to JDK 1.2 
(1998) so changing the behavior now would be risky. Hard to know how 
many custom class loaders are using this method but expanding the 
apiNote to include a snippet that catches IAE to deal with concurrent 
attempts to define the package would be okay.


As to your "all today's wishes & needs" comment. The "Package Version 
Specification" was of its time. It might be interesting to see how 
definePackage is used but I wouldn't be surprised to see the specXXX and 
implXXX parameters specified as null. Also the notion of sealed packages 
is legacy now.


-Alan

Re: RFR: 8355954: File.delete removes read-only files (win) [v7]

[Alan Bateman]

On Mon, 19 May 2025 17:10:50 GMT, Brian Burkhalter  wrote:

>> This change proposes to modify `java.io.File.delete()` so that regular files 
>> on Windows will not be deleted by default if their read-only attribute is 
>> set. A boolean-valued system compatibility property 
>> `jdk.io.File.deleteReadOnly` is defined to reinstate legacy behavior if 
>> desired.
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8355954: Make test Windows-only

Marked as reviewed by alanb (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/24977#pullrequestreview-2851532693

Re: RFR: 8356995: Provide default methods min(T, T) and max(T, T) in Comparator interface

[Roger Riggs]

On Mon, 19 May 2025 07:25:17 GMT, Tagir F. Valeev  wrote:

> Implementation of Comparator.min and Comparator.max methods. Preliminary 
> discussion is in this thread:
> https://mail.openjdk.org/pipermail/core-libs-dev/2025-May/145638.html
> The specification is mostly composed of Math.min/max and Collections.min/max 
> specifications. 
> 
> The methods are quite trivial, so I don't think we need more extensive 
> testing (e.g., using different comparators). But if you have ideas of new 
> useful tests, I'll gladly add them.
> 
> I'm not sure whether we should specify exactly the behavior in case if the 
> comparator returns 0. I feel that it could be a useful invariant that 
> `Comparator.min(a, b)` and `Comparator.max(a, b)` always return different 
> argument, partitioning the set of {a, b} objects (even if they are equal). 
> But I'm open to suggestions here.

Min and Max for the equals case would be more consistent if the value returned 
for equals was the same as the condition, as if `=>` or `<=` were used for the 
comparison.
The code would be easier to read and match the description if max used `>=` and 
min used `<=`.

-

PR Review: https://git.openjdk.org/jdk/pull/25297#pullrequestreview-2851547425

Re: RFR: 8356891: Some code simplifications for basic BigIntegers' bit operations [v6]

[Raffaello Giulietti]

On Mon, 19 May 2025 17:16:58 GMT, fabioromano1  wrote:

>> I think the suggestion is fine.
>
> Yes, but perhaps in the current version it's more clear what is the returned 
> value according to the condition.

One could see the suggested variant as "return magBitLength(), except for a 
corrective term in case blah blah..."

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25166#discussion_r2096209014

Re: RFR: 8356891: Some code simplifications for basic BigIntegers' bit operations [v6]

[fabioromano1]

On Mon, 19 May 2025 17:34:16 GMT, Raffaello Giulietti  
wrote:

>> Yes, but perhaps in the current version it's more clear what is the returned 
>> value according to the condition.
>
> One could see the suggested variant as "return magBitLength(), except for a 
> corrective term in case blah blah..."

Moreover, the current version avoids a subtraction in almost all cases, 
although probably it is negligible.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/25166#discussion_r2096217698