On Tue, 6 Sep 2022 14:24:43 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> more refactoring.
>
> src/java.base/share/classes/java/security/Security.java line 861:
>
>> 859: // check required values
>> 860: if (serviceName.isEmpty() || algName.isEmpty() ||
>> 861: (!attrValue.isEmpty() && attrName.isEmpty())) {
>
> Can we move the `attrName.isEmpty()` check inside the `else` block above? I
> find it easier to understand.
Sure, makes sense.
> src/java.base/share/classes/java/security/Security.java line 865:
>
>> 863: }
>> 864: }
>> 865: void apply(LinkedList<Provider> candidates) {
>
> `candidates.removeIf(p -> !isCriterionSatisfied(p));`
Ok.
> src/java.base/share/classes/java/security/Security.java line 895:
>
>> 893: if (standardName != null) {
>> 894: key = serviceName + "." + standardName +
>> 895: (attrName != null ? ' ' + attrName : "");
>
> I'd rather use `(' ' + attrName)`. Always not sure about operation precedence
> when `?:` is used.
Ok.
> src/java.base/share/classes/java/security/Security.java line 947:
>
>> 945: if (isKnownComposite(attribute)) {
>> 946: value = value.toUpperCase();
>> 947: prop = prop.toUpperCase();
>
> Use `toUpperCase(Locale.ENGLISH)`.
Sure~
-------------
PR: https://git.openjdk.org/jdk/pull/10008
