On Fri, 8 Mar 2024 21:56:19 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Hai-May Chao 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 six additional
>> commits since the last revision:
>>
>> - Merge
>> - remove unneeded checks in engineGetEntry
>> - Update engineDeleteEntry
>> - Update engineIsKeyEntry and engineIsCertificateEntry
>> - Update bug number in the test
>> - 8327461: KeyStore getEntry is not thread-safe
>
> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 301:
>
>> 299: Entry entry = entries.get(alias.toLowerCase(Locale.ENGLISH));
>> 300: Key entryKey = internalGetKey(entry, password);
>> 301: return entryKey;
>
> Combine the 2 lines above.
Done.
> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 480:
>
>> 478: Entry entry = entries.get(alias.toLowerCase(Locale.ENGLISH));
>> 479: Certificate[] certChain = internalGetCertificateChain(entry);
>> 480: return certChain;
>
> Combine the two lines above.
Done.
> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1027:
>
>> 1025:
>> 1026: Entry entry =
>> entries.remove(alias.toLowerCase(Locale.ENGLISH));
>> 1027: if (entry != null) {
>
> No need to check `entry != null`.
The API doc states: Returns: the previous value associated with key, or null if
there was no mapping for key. It’d be a good practice to check if the entry is
not null before proceeding with further operations. Would you please elaborate
why it is not needed here?
> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1082:
>
>> 1080: }
>> 1081:
>> 1082: private boolean internalEngineIsKeyEntry(Entry entry) {
>
> No need to have both `internal` and `Engine` in the method name. Use
> `internal` only to follow the other method names. Same with
> `internalEngineIsCertificateEntry` below.
Ok.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518399957
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518399969
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518399996
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518400012
