On Wed, 26 Jul 2023 16:03:31 GMT, John Jiang <jji...@openjdk.org> 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
