On Tue, 11 Feb 2025 16:35:35 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 58 commits: >> >> - Merge branch 'pem-merge' into pem >> - merge >> - Merge in PEMRecord as part of the API. >> - Merge branch 'pem-record' into pem-merge >> >> # Conflicts: >> # src/java.base/share/classes/java/security/PEMDecoder.java >> # src/java.base/share/classes/java/security/PEMRecord.java >> # src/java.base/share/classes/sun/security/util/Pem.java >> # test/jdk/java/security/PEM/PEMData.java >> # test/jdk/java/security/PEM/PEMDecoderTest.java >> # test/jdk/java/security/PEM/PEMEncoderTest.java >> - Merge branch 'master' into pem-record >> >> # Conflicts: >> # src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java >> - test fixes & cleanup >> - Implement stream decoding >> fix StringBuffer/Builder >> X509C* changes >> - Reorg tests data >> minor fixes >> - merge from pem branch >> - Merge branch 'pem' into pem-record >> >> # Conflicts: >> # src/java.base/share/classes/java/security/PEMEncoder.java >> # src/java.base/share/classes/sun/security/provider/X509Factory.java >> # src/java.base/share/classes/sun/security/util/Pem.java >> # test/jdk/java/security/PEM/PEMDecoderTest.java >> # test/jdk/java/security/PEM/PEMEncoderTest.java >> - ... and 48 more: https://git.openjdk.org/jdk/compare/22845a77...cc952c0b > > src/java.base/share/classes/java/security/PEMDecoder.java line 58: > >> 56: * </pre> >> 57: * >> 58: * A specified return class must extend {@link DEREncodable} and be an > > Suggest rewording as "Objects that are decoded and returned must extend ..." I think the suggestion changes the meaning a bit. The original text is about the specified return class `decode(String, **Class<DEREncodable>**)`, while the proposed text to me reads about the return value itself. > src/java.base/share/classes/java/security/PEMDecoder.java line 68: > >> 66: * available. >> 67: * >> 68: * <p> A new immutable {@code PEMDecoder} instance is created by using > > s/using/calling/ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 78: > >> 76: * >> 77: * <p> {@code String} values returned by this class use character set >> 78: * {@link java.nio.charset.StandardCharsets#ISO_8859_1 ISO-8859-1} > > Missing period at end of sentence. ok > src/java.base/share/classes/java/security/PEMDecoder.java line 199: > >> 197: * Decodes and returns {@link DEREncodable} from the given string. >> 198: * >> 199: * @param str PEM data in a String. > > Remove the period at end. Same comment applies to other @param, @return and > @throws descriptions. See > https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#@param > for more details where it says "End the phrase with a period only if another > phrase or sentence follows it." I'll look through them. > src/java.base/share/classes/java/security/PEMDecoder.java line 199: > >> 197: * Decodes and returns {@link DEREncodable} from the given string. >> 198: * >> 199: * @param str PEM data in a String. > > Suggest rewording as "a String containing PEM data". ok > src/java.base/share/classes/java/security/PEMDecoder.java line 200: > >> 198: * >> 199: * @param str PEM data in a String. >> 200: * @return an {@code DEREncodable} generated from the PEM data. > > s/an/a/ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 218: > >> 216: * {@code InputStream}. >> 217: * >> 218: * <p>The method will read the {@code InputStream} until PEM data is > > s/The/This/ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 374: > >> 372: * Configures and returns a new {@code PEMDecoder} instance from the >> 373: * current instance that will use KeyFactory and CertificateFactory >> classes >> 374: * from the specified {@link Provider}. Any errors using the > > What if `KeyFactory` and `CertificateFactory` are in different providers? Do > we want to have a method that also takes two provider parameters? I think multiple provider parameters doesn't make usability easier and adds project code complexity. In the case you describe, the user knows their providers factory support and they can just create a new PEMDecoder instance for each. If they do not know, then they don't use this method. > src/java.base/share/classes/java/security/PEMDecoder.java line 377: > >> 375: * {@code provider} will occur during decoding. >> 376: * >> 377: * <p>If {@code params} is {@code null}, a new instance is returned >> with > > There is no variable named `params` - do you mean `provider`? Also, why not > throw an NPE and not allow a `null` provider, since it would be the same as > calling `of()`? It was meant to be `provider`. I didn't think checking for null and throwing an exception is necessary when it is the default operation. This shows up when coding with a provider variable with unknown value. In this case, the app doesn't have to check if `provider == null` so that it can call `of()`. Unfortunately this situation is common with `getInstance()` which result in a lot of if-else statements. > src/java.base/share/classes/java/security/spec/EncodedKeySpec.java line 52: > >> 50: >> 51: private final byte[] encodedKey; >> 52: private String algorithmName; > > I think this can be marked `final` now. ok > src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 200: > >> 198: DerValue bits = >> value.withTag(DerValue.tag_BitString); >> 199: //byte[] bytes = bits.getBitString(); >> 200: //BitArray bitArray = new BitArray(bytes[0] * 8 - >> 2, bytes, 3); > > Commented out code, remove? already removed > src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 207: > >> 205: pubKeyEncoded = new X509Key(algid, >> 206: bits.getUnalignedBitString()).getEncoded(); >> 207: */ > > Commented out code, remove? already removed ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052669236 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052680351 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052693549 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052698537 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052698603 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052698711 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052699218 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049720785 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049684721 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052656953 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052649026 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052649253
