On Tue, 13 May 2025 22:13:13 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> comments
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 61:
>
>> 59: * </pre>
>> 60: *
>> 61: * <p> If the PEM does not have a JCE object representation, it returns a
>
> Not "it", but the two methods.
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 69:
>
>> 67: * methods are useful when extracting or changing the return class.
>> 68: * The Class parameter can specify the returned
>> 69: * key object from a PEM containing a public and private key. If only
>
> Not always "key". If this is meant to be an example, prepend with "For
> example".
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 89:
>
>> 87: * decoder not configured for decryption, an {@link
>> EncryptedPrivateKeyInfo}
>> 88: * object is returned. {@code EncryptedPrivateKeyInfo} methods must be
>> used to
>> 89: * retrieve the {@link PrivateKey}.
>
> Do we need this last sentence? "EncryptedPrivateKeyInfo methods must be used
> to...".
maybe not
> src/java.base/share/classes/java/security/PEMDecoder.java line 97:
>
>> 95: * PEMDecoder pd = PEMDecoder.of();
>> 96: * PrivateKey priKey = pd.decode(priKeyPEM, PrivateKey.class);
>> 97: * }
>
> Add examples on `withFactory` and `withDecryption`.
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 133:
>
>> 131: * Returns an instance of {@code PEMDecoder}.
>> 132: *
>> 133: * @return a new {@code PEMDecoder} instance
>
> Not always new.
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 234:
>
>> 232: * {@code leadingData}.
>> 233: *
>> 234: * <p>If no PEM data is found, an {@code IllegalArgumentException}
>> is
>
> Duplicated sentence. Or do you mean to say the same exception is thrown if
> decoding fails?
no
> src/java.base/share/classes/java/security/PEMDecoder.java line 281:
>
>> 279: * @param is InputStream containing PEM data
>> 280: * @return a {@code DEREncodable}
>> 281: * @throws IOException on IO error with the {@code InputStream}
>
> Since IOE is not purely IO error and contains format errors that are not
> recoverable. Do you want to say more?
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 283:
>
>> 281: * @throws IOException on IO error with the {@code InputStream}
>> 282: * @throws EOFException the end of {@code InputStream} has been
>> 283: * unexpectedly reached.
>
> Not really "unexpected". It looks like every program that tries to read
> multiple PEM blocks will encounter this.
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 285:
>
>> 283: * unexpectedly reached.
>> 284: * @throws IllegalArgumentException on error in decoding or no PEM
>> data
>> 285: * found
>
> "no PEM data found". Isn't that EOFException?
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 312:
>
>> 310: * {@link java.nio.charset.StandardCharsets#UTF_8 UTF-8}.
>> 311: *
>> 312: * <p> For all other class parameters, {@code
>> IllegalArgumentException} is
>
> "For all other class parameters", you mean not `PEMRecord`? There is a
> paragraph in between (UTF-8 thingy) and users have forgotten what "other"
> means.
paragraph is unnecessary
> src/java.base/share/classes/java/security/PEMDecoder.java line 325:
>
>> 323: * @throws NullPointerException when any input values are null.
>> 324: *
>> 325: * @see PEMDecoder for how {@code tClass} can be used.
>
> Since it's a `@see`, let's make the title a little more formal. Maybe add a
> header (`<h2>`) in the spec.
actually I don't think this like is necessary
> src/java.base/share/classes/java/security/PEMEncoder.java line 127:
>
>> 125:
>> 126: /**
>> 127: * Returns a new instance of {@code PEMEncoder}.
>
> Not always new.
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 50:
>
>> 48: * <p> During the instantiation of this record, there is no validation
>> 49: * for the {@code type} or {@code pem}. {@code leadingData} is not
>> 50: * defensively copied.
>
> Not only instantiation, but the data is also not defensively copied when
> someone calls `leadingData`. Do we need to mention this as well?
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 76:
>
>> 74: * before the PEM header. This value maybe
>> {@code null}.
>> 75: * @throws IllegalArgumentException if the {@code type} is
>> incorrectly
>> 76: * formatted.
>
> Is there a place defining what is a correctly formatted `type`?
The class definition for `type` given an example.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088189751
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088192299
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088202146
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088358703
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088209096
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088209735
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088232122
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088251848
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088281449
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088296864
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088290091
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088299845
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087829601
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087834756
