On Wed, 7 May 2025 18:38:31 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains 66 commits: >> >> - major code review comments update >> - Merge branch 'master' into pem >> - Merge branch 'master' into pem >> - javadoc updates >> - code review comments >> - merge with master >> - better comment and remove commented out code >> - Merge branch 'master' into pem >> - Merge branch 'pem-merge' into pem >> - merge >> - ... and 56 more: https://git.openjdk.org/jdk/compare/e2ae50d8...0c540327 > > src/java.base/share/classes/java/security/PEMDecoder.java line 68: > >> 66: * class is specified. >> 67: * >> 68: * <p> A new immutable {@code PEMDecoder} instance is created when >> configured > > no need to say "immutable" as first sentence says all PEMDecoders are > immutable. ok > src/java.base/share/classes/java/security/PEMDecoder.java line 71: > >> 69: * with {@linkplain #withFactory} and/or {@linkplain #withDecryption}. >> 70: * Configuring an instance for decryption does not prevent decoding with >> 71: * unencrypted PEM. Any encrypted PEM that does not use the configured >> password > > I found the last 3 sentences a bit jumping too quickly into details. I > suggest having 2 sentences after the first one, where each sentence explains > what the `withDecryption` and `withFactory` methods do, respectively. I split them and reworded it, HTH. > src/java.base/share/classes/java/security/PEMDecoder.java line 92: > >> 90: * @spec https://www.rfc-editor.org/info/rfc7468 >> 91: * RFC 7468: Textual Encodings of PKIX, PKCS, and CMS Structures >> 92: * > > Might be useful to add an `@see PEMEncoder` (and vice-versa). ok. I added a link > src/java.base/share/classes/java/security/PEMDecoder.java line 228: > >> 226: * <p>This method will read the {@code InputStream} until PEM data >> is >> 227: * found or until the end of the stream. Non-PEM data in the >> 228: * {@code InputStream} before the PEM header will be ignored by the >> decoder. > > s/will be/is/ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 230: > >> 228: * {@code InputStream} before the PEM header will be ignored by the >> decoder. >> 229: * If only non-PEM data is found a {@link PEMRecord} is returned >> with that >> 230: * data. > > Why? Shouldn't this be an exception as it isn't PEM? I think you're right. > src/java.base/share/classes/java/security/PEMDecoder.java line 252: > >> 250: * <p> >> 251: * {@code tClass} can be used to change the return type instance: >> 252: * <ul> > > This list feels more like programming advice. I would suggest either removing > this, or putting it in the class description but not as a bulleted list, > instead use complete sentences. Rewriting sounds fine. > src/java.base/share/classes/java/security/PEMDecoder.java line 399: > >> 397: >> 398: /** >> 399: * Returns a new {@code PEMDecoder} instance from the current >> instance > > I find the words "a new instance from a current instance" a little unusual. > > It might be simpler and clearer to say "Returns a copy of this PEMDecoder > that will decrypt encrypted PEM data such as encrypted private keys with the > specified password." > > (I don't feel it is necessary to mention that it still also decodes > unencrypted PEM - that seems implied to me). The rewrite looks good, but I want to keep the comment about unencrypted PEM. PEMEncoder only encrypts PrivateKey and rejects any other DEREncodable. I think it's worth mentioning. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078839633 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078849405 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078855537 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078876102 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078948310 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080055983 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078969843
