On Thu, 25 Jul 2024 15:42:59 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - bad test check
>> - internal PEMRecord optimization
>
> src/java.base/share/classes/java/security/Key.java line 1:
>
>> 1: /*
>
> This file is not modified.
Same as EKS :(
> src/java.base/share/classes/java/security/PEMEncoder.java line 88:
>
>> 86:
>> 87: // If non-null, encoder is configured for encryption
>> 88: private Cipher cipher = null;
>
> Is it worth creating `cipher` lazily? Also, why does `PEMDecoder` allows
> setting a factory but not here?
if cipher is defined this is an encrypted PEMEncoder instance, so yes it's
important.
Encoding doesn't need a factory when the object already provides the DER
encoding.
> src/java.base/share/classes/java/security/spec/EncodedKeySpec.java line 32:
>
>> 30:
>> 31: import java.io.IOException;
>> 32: import java.security.DEREncodable;
>
> Useless import.
Yes, probably change that didn't get completely undone
> src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 109:
>
>> 107: throws InvalidKeyException {
>> 108: this(privEncoding);
>> 109: pubKeyEncoded = pubEncoding;
>
> So if there is already a public key in `privEncoding`, it will be
> overwritten? BTW, it seems this method is not used anywhere.
If it isn't used anywhere, then it's probably from an old idea that I didn't
completely clean up
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692459154
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692458145
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692458787
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1692461176
