On Wed, 8 Jun 2022 15:53:06 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 11 commits: >> >> - Merge >> - fourth iteration >> - Merge >> - Merge >> - third iteration >> - Merge >> - Merge >> - second iteration >> - Merge >> - Merge >> - ... and 1 more: https://git.openjdk.org/jdk/compare/4d6fb515...44140a01 > > src/java.base/share/classes/java/security/Provider.java line 1098: > >> 1096: boolean matches(String type, String algorithm) { >> 1097: return (Objects.equals(this.type, type)) && >> 1098: (Objects.equals(this.originalAlgorithm, algorithm)); > > In fact, I'm not sure why the original code is brave enough to use `==` > instead of `equals`. If you do believe it's a real bug (and can write a > regression test), let's fix it in a real bug fix. Otherwise, keep using `==` > and add a comment saying it's correct. The `ServiceKey` class has `equals` and `matches` methods. The `matches` method returns true if two objects contain identical String pointers. The `equals` method returns true if String pointers are different but have the same value. This is by design, so it would be a mistake to take IntelliJ's suggestion here. Keeping the `==` and adding a comment. Good catch. > src/java.base/share/classes/java/security/SecureClassLoader.java line 221: > >> 219: // only), and the fragment is not considered. >> 220: CodeSourceKey key = new CodeSourceKey(cs); >> 221: /* not used */ > > What is not used? `key1`? How about just rename it to `unused`? Renamed `key1` to `unused`. > src/java.base/share/classes/java/security/SecureClassLoader.java line 235: > >> 233: } >> 234: >> 235: private record CodeSourceKey(CodeSource cs) { > > Really necessary? No. It was just an IntelliJ suggestion. I have no preference. > src/java.base/share/classes/java/security/SecureRandom.java line 905: > >> 903: private static final Pattern pattern = >> 904: Pattern.compile( >> 905: "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?"); > > Don't quite understand this change. Can you explain? The author must have thought `:` and `,` were metacharacters that needed to be escaped to turn them into ordinary characters. I verified with a small java program that` \:` and` \,` are equivalent to `;` and `,`. The IntelliJ complaint is "Redundant character escape `'\:'`". > src/java.base/share/classes/java/security/UnresolvedPermission.java line 369: > >> 367: this.certs != null && that.certs == null || >> 368: this.certs != null && >> 369: this.certs.length != that.certs.length) { > > I see you keep a lot of parentheses in other places. For example, `(!r)`. This is an IntelliJ suggestion. I don't know why it complains about some cases and ignores others. Perhaps it has something to do with readability. > src/java.base/share/classes/java/security/cert/CertPathValidator.java line > 335: > >> 333: String cpvtype = >> 334: AccessController.doPrivileged((PrivilegedAction<String>) () >> -> >> 335: Security.getProperty(CPV_TYPE)); > > See too many, maybe we need a dedicated method in > `sun.security.util.SecurityProperties`? A privilegedGetProperty method? That would be the subject of a new bug I think. ------------- PR: https://git.openjdk.org/jdk/pull/8319
