On Mon, 25 Sep 2023 03:40:47 GMT, Mark Powers <mpow...@openjdk.org> wrote:
>> https://bugs.openjdk.org/browse/JDK-8315042 > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > comment from Weijun test/jdk/sun/security/x509/X509CRLImpl/UnexpectedNPE.java line 27: > 25: * @test > 26: * @library /test/lib > 27: * @bug 5052433 8315042 Usually we put `@bug` before `@library`. test/jdk/sun/security/x509/X509CRLImpl/UnexpectedNPE.java line 39: > 37: public class UnexpectedNPE { > 38: CertificateFactory cf = null ; > 39: private static final String in = > "MAsGCSqGSMP7TQEHAjI1Bgn///////8wCwUyAQ=="; Just embed the string inside the definition of `decodedBytes`. No need to create a field for it. test/jdk/sun/security/x509/X509CRLImpl/UnexpectedNPE.java line 41: > 39: private static final String in = > "MAsGCSqGSMP7TQEHAjI1Bgn///////8wCwUyAQ=="; > 40: > 41: public UnexpectedNPE() {} Useless constructor. test/jdk/sun/security/x509/X509CRLImpl/UnexpectedNPE.java line 47: > 45: byte[] encoded_2 = { 0x30, 0x01, 0x00, 0x00 }; > 46: byte[] encoded_3 = { 0x30, 0x01, 0x00 }; > 47: byte[] decodedBytes = Base64.getDecoder().decode(in); Maybe rename to `encoded_4`? test/jdk/sun/security/x509/X509CRLImpl/UnexpectedNPE.java line 49: > 47: byte[] decodedBytes = Base64.getDecoder().decode(in); > 48: > 49: UnexpectedNPE unpe = new UnexpectedNPE() ; Just modify `run` to static. No class instance needed. test/jdk/sun/security/x509/X509CRLImpl/UnexpectedNPE.java line 60: > 58: if (cf == null) { > 59: try { > 60: cf = CertificateFactory.getInstance("X.509", "SUN"); Make `cf` static and move the line above to `main`. Or, make it a local variable in `main` and pass it to all `run` calls. test/jdk/sun/security/x509/X509CRLImpl/UnexpectedNPE.java line 76: > 74: > 75: System.out.println("CRLException has not been thrown"); > 76: return false; I think only `generateCRLs` is enough here since neither bug is about X.509 encoding. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15844#discussion_r1336058841 PR Review Comment: https://git.openjdk.org/jdk/pull/15844#discussion_r1336054466 PR Review Comment: https://git.openjdk.org/jdk/pull/15844#discussion_r1336055352 PR Review Comment: https://git.openjdk.org/jdk/pull/15844#discussion_r1336055601 PR Review Comment: https://git.openjdk.org/jdk/pull/15844#discussion_r1336055142 PR Review Comment: https://git.openjdk.org/jdk/pull/15844#discussion_r1336055988 PR Review Comment: https://git.openjdk.org/jdk/pull/15844#discussion_r1336058145
