RFR: 8313226: Redundant condition test in X509CRLImpl

2023-07-26 Thread John Jiang 
if ((nextByte == DerValue.tag_SequenceOf)
&& (! ((nextByte & 0x0c0) == 0x080))) {
...
...
}

If `nextByte` is `DerValue.tag_SequenceOf`, exactly `0x30`, then the test after 
`&&` should always be true.

-

Commit messages:
 - 8313226: Redundant condition test in X509CRLImpl

Changes: https://git.openjdk.org/jdk/pull/15051/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=15051=00
  Issue: https://bugs.openjdk.org/browse/JDK-8313226
  Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/15051.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15051/head:pull/15051

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

Withdrawn: 8312630: java/security should not create unmodifiable collections with redundant wrapping

2023-07-26 Thread John Jiang 
On Tue, 25 Jul 2023 06:27:25 GMT, John Jiang  wrote:

> Some java/security classes apply the below coding style,
> 
> Set set = ...;
> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
> 
> It may be unnecessary to wrap that `set` with HashSet before creating 
> `unmodifiableSet`.
> Some usages on `Collections.unmodifiableList` and 
> `Collections.unmodifiableMap` have the same issue.

This pull request has been closed without being integrated.

-

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

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping [v2]

2023-07-26 Thread John Jiang 
On Wed, 26 Jul 2023 16:52:20 GMT, Sean Mullan  wrote:

> The latter part is true (prevented from subsequent modification) but, unless 
> I am mistaken, the former (making a clone/copy) is not. For example, before 
> your change, this assert would pass:
> 
> ```
> Map m = Collections.unmodifiableMap(map);
> DomainLoadStoreParameters params = new DomainLoadStoreParameters(uri, m);
> assert m != params.getProtectionParams();
> ```
> 
> After your change, I think it fails (can you check?). Even though the 
> protection in both cases should be adequate, it is a subtle behavior change 
> that I don't think the current specification covers.

I get the point now.

> As mentioned, I don't think this change is critical unless you have a 
> stronger case.

I'll withdraw this PR.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275577482

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping

2023-07-26 Thread Xue-Lei Andrew Fan 
On Wed, 26 Jul 2023 06:11:56 GMT, Xue-Lei Andrew Fan  wrote:

>> Some java/security classes apply the below coding style,
>> 
>> Set set = ...;
>> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
>> 
>> It may be unnecessary to wrap that `set` with HashSet before creating 
>> `unmodifiableSet`.
>> Some usages on `Collections.unmodifiableList` and 
>> `Collections.unmodifiableMap` have the same issue.
>
> Note: Please don't backport this update unless 
> [JDK-6323374](https://bugs.openjdk.org/browse/JDK-6323374) is backport as 
> well.

> @XueleiFan Thanks for your review and more infos!
> 
> I just dug a bit history and found 
> [JDK-8258514](https://bugs.openjdk.org/browse/JDK-8258514) applied 
> `List::copyOf` instead of `Collections::unmodifiableList`. Now, I also follow 
> this way and use `List/Set/Map::copyOf`.

The use of copyOf for the update is mainly because the 
Collections::unmodifiableList was not updated yet at that time.  It could be 
simpler to keep the behavior consistent by using Collections::unmodifiableList 
in this update.  Otherwise, more effect may be required to check if the use of 
copyOf could change the behaviors.

-

PR Comment: https://git.openjdk.org/jdk/pull/15008#issuecomment-1652370187

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping [v2]

2023-07-26 Thread Sean Mullan 
On Wed, 26 Jul 2023 16:03:31 GMT, John Jiang  wrote:

> > it says "It is cloned to prevent subsequent modification."
> > Now, if the set passed in is unmodifiable, that is no longer true.
> 
> My understanding: The parameter `protectionParams` is cloned as 
> `this.protectionParams`, and `this.protectionParams` should be prevented from 
> subsequent modification.

The latter part is true (prevented from subsequent modification) but, unless I 
am mistaken, the former (making a clone/copy) is not. For example, before your 
change, this assert would pass:


Map m = Collections.unmodifiableMap(map);
DomainLoadStoreParameters params = new DomainLoadStoreParameters(uri, m);
assert m != params.getProtectionParams();


After your change, I think it fails (can you check?). Even though the 
protection in both cases should be adequate, it is a subtle behavior change 
that I don't think the current specification covers.

> 
> `Map::copyOf` looks meet this requirement, no matter the parameter 
> `protectionParams` is modifiable or not.
> 
> > It also will now throw NPE if any of the keys or values are null, whereas 
> > before it did not,
> > and the specification does not specify that in the @throws clause.
> 
> Yes, this is a problem. I don't want to change any behavior.

As mentioned, I don't think this change is critical unless you have a stronger 
case.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275244650

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping [v2]

2023-07-26 Thread Sean Mullan 
On Wed, 26 Jul 2023 09:28:46 GMT, John Jiang  wrote:

>> Some java/security classes apply the below coding style,
>> 
>> Set set = ...;
>> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
>> 
>> It may be unnecessary to wrap that `set` with HashSet before creating 
>> `unmodifiableSet`.
>> Some usages on `Collections.unmodifiableList` and 
>> `Collections.unmodifiableMap` have the same issue.
>
> John Jiang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use copyOf instead of modifiableXXX

src/java.base/share/classes/java/security/KeyStore.java line 769:

> 767: }
> 768: this.cert = trustedCert;
> 769: this.attributes = Set.copyOf(attributes);

Similar comments.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275083599

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Brian Burkhalter 
On Tue, 25 Jul 2023 23:50:07 GMT, Brian Burkhalter  wrote:

>> Limit native memory allocation and move write loop from the native layer 
>> into Java. This change should make the OOME reported in the issue much less 
>> likely.
>
> Brian Burkhalter has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - 6478546: Decrease malloc limit to 1.5 MB
>  - 6478546: Minor refactoring
>  - 6478546: Prevent short read

The EOF handling in the above should be

if (n < 0) {
// EOF
if (nread == 0)
return -1;
break;
}

or zero will be returned if EOF is encountered on the first read (I made the 
same mistake in code that was not checked in).

-

PR Comment: https://git.openjdk.org/jdk/pull/14981#issuecomment-1652242356

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Brian Burkhalter 
On Tue, 25 Jul 2023 23:50:07 GMT, Brian Burkhalter  wrote:

>> Limit native memory allocation and move write loop from the native layer 
>> into Java. This change should make the OOME reported in the issue much less 
>> likely.
>
> Brian Burkhalter has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - 6478546: Decrease malloc limit to 1.5 MB
>  - 6478546: Minor refactoring
>  - 6478546: Prevent short read

> The changes in 
> [69941de](https://github.com/openjdk/jdk/commit/69941de4aa27ee34d957621d73c77942237c33cd)
>  are also using nested blockers, this is benign, but if this code is change 
> then they can be dropped from the callers.

I don't see where this is happening.

-

PR Comment: https://git.openjdk.org/jdk/pull/14981#issuecomment-1652223466

Integrated: 8309088: security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java fails

2023-07-26 Thread Rajan Halade 
On Mon, 24 Jul 2023 15:13:42 GMT, Rajan Halade  wrote:

> This is an update to test certificates issued by Amazon root CA. The new EE 
> certificates have CRLDP so I have enabled CRL check on those. This test is 
> updated to cover the failure while we wait for 
> [JDK-8308592](https://bugs.openjdk.org/browse/JDK-8308592)

This pull request has now been integrated.

Changeset: 4c2e54fb
Author:Rajan Halade 
URL:   
https://git.openjdk.org/jdk/commit/4c2e54fb055bee0af5cd838fdd32a0f7902d51e3
Stats: 491 lines in 1 file changed: 142 ins; 30 del; 319 mod

8309088: 
security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java 
fails

Reviewed-by: mullan

-

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

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Brian Burkhalter 
On Wed, 26 Jul 2023 15:58:15 GMT, Vyom Tewari  wrote:

>>> please reformat line 173
>> 
>> Why?
>
> "buf = (char*)malloc(len*sizeof(char));" --> "buf = (char*)malloc(len * 
> sizeof(char));"

> "buf = (char*)malloc(len_sizeof(char));" --> "buf = (char_)malloc(len * 
> sizeof(char));"

Both sides of this are wrong.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14981#discussion_r1275193937

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping [v2]

2023-07-26 Thread John Jiang 
On Wed, 26 Jul 2023 14:41:16 GMT, Sean Mullan  wrote:

> it says "It is cloned to prevent subsequent modification."
> Now, if the set passed in is unmodifiable, that is no longer true. 

My understanding:
The parameter `protectionParams` is cloned as `this.protectionParams`,
and `this.protectionParams` should be prevented from subsequent modification.

`Map::copyOf` looks meet this requirement, no matter the parameter 
`protectionParams` is modifiable or not.

> It also will now throw NPE if any of the keys or values are null, whereas 
> before it did not,
> and the specification does not specify that in the @throws clause.

Yes, this is a problem.
I don't want to change any behavior.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275191302

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Vyom Tewari 
On Wed, 26 Jul 2023 15:49:38 GMT, Brian Burkhalter  wrote:

>> src/java.base/share/native/libjava/io_util.c line 173:
>> 
>>> 171: if (len > MAX_MALLOC_SIZE)
>>> 172: len = MAX_MALLOC_SIZE;
>>> 173: buf = (char*)malloc(len*sizeof(char));
>> 
>> please reformat line 173
>
>> please reformat line 173
> 
> Why?

"buf = (char*)malloc(len*sizeof(char));" --> "buf = (char*)malloc(len * 
sizeof(char));"

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14981#discussion_r1275185140

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Brian Burkhalter 
On Wed, 26 Jul 2023 05:31:57 GMT, Vyom Tewari  wrote:

> please reformat line 173

Why?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14981#discussion_r1275174632

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Brian Burkhalter 
On Wed, 26 Jul 2023 09:29:18 GMT, Alan Bateman  wrote:

> I think the main issue with this version is that it changes behavior of the 
> read methods to work like "read fully", I don't think we should do that.

To me it looks more like `readNBytes`.

-

PR Comment: https://git.openjdk.org/jdk/pull/14981#issuecomment-1652080776

Re: RFR: 8309088: security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java fails

2023-07-26 Thread Sean Mullan 
On Mon, 24 Jul 2023 15:13:42 GMT, Rajan Halade  wrote:

> This is an update to test certificates issued by Amazon root CA. The new EE 
> certificates have CRLDP so I have enabled CRL check on those. This test is 
> updated to cover the failure while we wait for 
> [JDK-8308592](https://bugs.openjdk.org/browse/JDK-8308592)

Marked as reviewed by mullan (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/15000#pullrequestreview-1548060463

Integrated: 8313087: DerValue::toString should output a hex view of the values in byte array

2023-07-26 Thread John Jiang 
On Wed, 26 Jul 2023 04:02:04 GMT, John Jiang  wrote:

> DerValue::toString may be better to output the hex view of the byte array 
> variable `buffer`.

This pull request has now been integrated.

Changeset: 830413f1
Author:John Jiang 
URL:   
https://git.openjdk.org/jdk/commit/830413f19a6d998ff6c899c05e8fa93b6b2b0644
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod

8313087: DerValue::toString should output a hex view of the values in byte array

Reviewed-by: mullan

-

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

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Brian Burkhalter 
On Wed, 26 Jul 2023 05:26:52 GMT, Vyom Tewari  wrote:

>> Brian Burkhalter has updated the pull request incrementally with three 
>> additional commits since the last revision:
>> 
>>  - 6478546: Decrease malloc limit to 1.5 MB
>>  - 6478546: Minor refactoring
>>  - 6478546: Prevent short read
>
> src/java.base/share/native/libjava/io_util.c line 163:
> 
>> 161: }
>> 162: 
>> 163: if (outOfBounds(env, off, len, bytes)) {
> 
> we are already performing this check at Java level, do we need the perform 
> this check again ?

Please refer to my prior 
[comment](https://github.com/openjdk/jdk/pull/14981#issuecomment-1646312987).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14981#discussion_r1275120710

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping [v2]

2023-07-26 Thread Sean Mullan 
On Wed, 26 Jul 2023 09:28:46 GMT, John Jiang  wrote:

>> Some java/security classes apply the below coding style,
>> 
>> Set set = ...;
>> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
>> 
>> It may be unnecessary to wrap that `set` with HashSet before creating 
>> `unmodifiableSet`.
>> Some usages on `Collections.unmodifiableList` and 
>> `Collections.unmodifiableMap` have the same issue.
>
> John Jiang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   use copyOf instead of modifiableXXX

src/java.base/share/classes/java/security/DomainLoadStoreParameter.java line 
137:

> 135: }
> 136: this.configuration = configuration;
> 137: this.protectionParams = Map.copyOf(protectionParams);

This change is no longer strictly compliant with the specification. On line 
126, it says "It is cloned to prevent subsequent modification." Now, if the set 
passed in is unmodifiable, that is no longer true. It also will now throw NPE 
if any of the keys or values are null, whereas before it did not, and the 
specification does not specify that in the @throws clause.

So, this change would require specification changes and a CSR.

Can you provide more details on whether this is a real performance issue in 
practice? 

I would tend to avoid changes like this that affect the API specification 
unless there is a very strong compelling case where it improves performance for 
a real use case.

src/java.base/share/classes/java/security/KeyStore.java line 569:

> 567: }
> 568: 
> 569: this.attributes = Set.copyOf(attributes);

Similar comments and concerns as noted above in DomainLoadStoreParameter.

src/java.base/share/classes/java/security/KeyStore.java line 687:

> 685: }
> 686: this.sKey = secretKey;
> 687: this.attributes = Set.copyOf(attributes);

Similar comments again.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275080672
PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275082327
PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275082978

Re: RFR: 8313087: DerValue::toString should output a hex view of the values in byte array

2023-07-26 Thread John Jiang 
On Wed, 26 Jul 2023 13:49:49 GMT, Sean Mullan  wrote:

>> DerValue::toString may be better to output the hex view of the byte array 
>> variable `buffer`.
>
> Please change the title of the bug to be "DerValue::toString should output a 
> hex view of the values in byte array".
> 
> Otherwise, LGTM.

@seanjmullan 
Thanks for your review!

> Please change the title of the bug to be "DerValue::toString should output a 
> hex view of the values in byte array".

Done.

-

PR Comment: https://git.openjdk.org/jdk/pull/15029#issuecomment-1651932667

Re: RFR: 8313087: DerValue::toString should output the values in byte array

2023-07-26 Thread Sean Mullan 
On Wed, 26 Jul 2023 04:02:04 GMT, John Jiang  wrote:

> DerValue::toString may be better to output the hex view of the byte array 
> variable `buffer`.

Please change the title of the bug to be "DerValue::toString should output a 
hex view of the values in byte array".

Otherwise, LGTM.

-

Marked as reviewed by mullan (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15029#pullrequestreview-1547785664

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping

2023-07-26 Thread John Jiang 
On Wed, 26 Jul 2023 06:11:56 GMT, Xue-Lei Andrew Fan  wrote:

>> Some java/security classes apply the below coding style,
>> 
>> Set set = ...;
>> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
>> 
>> It may be unnecessary to wrap that `set` with HashSet before creating 
>> `unmodifiableSet`.
>> Some usages on `Collections.unmodifiableList` and 
>> `Collections.unmodifiableMap` have the same issue.
>
> Note: Please don't backport this update unless 
> [JDK-6323374](https://bugs.openjdk.org/browse/JDK-6323374) is backport as 
> well.

@XueleiFan 
Thanks for your review and more infos!

I just dug a bit history and found [JDK-8258514] applied `List::copyOf` instead 
of `Collections::unmodifiableList`.
Now, I also follow this way and use `List/Set/Map::copyOf`.

[JDK-8258514]:


-

PR Comment: https://git.openjdk.org/jdk/pull/15008#issuecomment-1651391963

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Alan Bateman 
On Wed, 26 Jul 2023 00:00:33 GMT, Brian Burkhalter  wrote:

> There was no dropping of the file system cache. I would think this would have 
> more of an effect on measuring the throughput of writing than reading.

In that case, I assume the benchmarks are just reading from the file system 
cache so there isn't any actual file I/O.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14981#discussion_r1274666144

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Alan Bateman 
On Tue, 25 Jul 2023 23:50:07 GMT, Brian Burkhalter  wrote:

>> Limit native memory allocation and move write loop from the native layer 
>> into Java. This change should make the OOME reported in the issue much less 
>> likely.
>
> Brian Burkhalter has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - 6478546: Decrease malloc limit to 1.5 MB
>  - 6478546: Minor refactoring
>  - 6478546: Prevent short read

I looked through the latest version (69941de4).

I think the main issue with this version is that it changes behavior of the 
read methods to work like "read fully", I don't think we should do that. To 
preserve long standing behavior it should attempt a second/subsequent read when 
the clamped buffer is filled. I think this is close to what you want here:

private int readBytes(byte[] b, final int off, int len) throws IOException {
Objects.checkFromIndexSize(off, len, b.length);
int nread = 0;
int pos = off;
int remaining = len;
do {
int size = Math.min(remaining, 64 * 1024);
try {
int n = readBytes0(b, pos, size);
if (n < 0) {
// EOF
break;
}
nread += n;
if (n < size) {
// buffer not filled
break;
}
pos += n;
remaining -= n;
} catch (IOException ioe) {
if (nread > 0) {
break;
}
throw ioe;
}
} while (remaining > 0);
return nread;
}


The changes in 69941de4 are also using nested blockers, this is benign, but if 
this code is change then they can be dropped from the callers.

-

PR Comment: https://git.openjdk.org/jdk/pull/14981#issuecomment-1651352313

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping [v2]

2023-07-26 Thread John Jiang 
> Some java/security classes apply the below coding style,
> 
> Set set = ...;
> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
> 
> It may be unnecessary to wrap that `set` with HashSet before creating 
> `unmodifiableSet`.
> Some usages on `Collections.unmodifiableList` and 
> `Collections.unmodifiableMap` have the same issue.

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

  use copyOf instead of modifiableXXX

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/15008/files
  - new: https://git.openjdk.org/jdk/pull/15008/files/64f607da..b8f2d9e0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=15008=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=15008=00-01

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

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

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Alan Bateman 
On Wed, 26 Jul 2023 06:17:13 GMT, Vyom Tewari  wrote:

> If i am reading code correctly then with the new implementation, until client 
> issue the next "FIS.read" he may or may not know if there was exception 
> pending in previous 'read' call ?.
> I am not sure in case of partial read we have suppress the exception.

It's a forced move because bytes have already been copied into user's byte[].

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14981#discussion_r1274438315

Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available [v2]

2023-07-26 Thread Vyom Tewari 
On Tue, 25 Jul 2023 23:50:07 GMT, Brian Burkhalter  wrote:

>> Limit native memory allocation and move write loop from the native layer 
>> into Java. This change should make the OOME reported in the issue much less 
>> likely.
>
> Brian Burkhalter has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - 6478546: Decrease malloc limit to 1.5 MB
>  - 6478546: Minor refactoring
>  - 6478546: Prevent short read

src/java.base/share/classes/java/io/FileInputStream.java line 266:

> 264: } catch (IOException e) {
> 265: // Throw only if no bytes have been read
> 266: if (off == start)

If i am reading code correctly then with the new implementation, until client 
issue the next "FIS.read" he may or may not know if there was exception pending 
in previous 'read' call ?.
I am not sure in case of partial read we have suppress the exception.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14981#discussion_r1274424360

Re: RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping

2023-07-26 Thread Xue-Lei Andrew Fan 
On Tue, 25 Jul 2023 06:27:25 GMT, John Jiang  wrote:

> Some java/security classes apply the below coding style,
> 
> Set set = ...;
> Set unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
> 
> It may be unnecessary to wrap that `set` with HashSet before creating 
> `unmodifiableSet`.
> Some usages on `Collections.unmodifiableList` and 
> `Collections.unmodifiableMap` have the same issue.

Marked as reviewed by xuelei (Reviewer).

Note: Please don't backport this update unless 
[JDK-6323374](https://bugs.openjdk.org/browse/JDK-6323374) is backport as well.

-

PR Review: https://git.openjdk.org/jdk/pull/15008#pullrequestreview-1546896379
PR Comment: https://git.openjdk.org/jdk/pull/15008#issuecomment-1651039887