On Fri, 23 Oct 2020 16:34:54 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Do you want to refactor some of the classes where `HexFormat` would be a >> well-suited replacement for the current code, and what do you think about my >> other previous review comments (mostly formatting related)? >> The bridgekeeper bot was so kind and censored my [previous >> comment](https://github.com/openjdk/jdk/pull/482#issuecomment-710709003), >> but apparently it can't handle review comments yet ... >> >> And no, I am not going to sign the [OpenJDK Terms of >> Use](https://openjdk.java.net/legal/tou/terms) for a simple pull request >> comment, especially when the Terms of Use document is quite long and >> contains the following: >>> You hereby grant to Oracle and all Users a royalty-free, perpetual, >>> irrevocable, worldwide, non-exclusive and fully sub-licensable right and >>> license under Your intellectual property rights to reproduce, modify, >>> adapt, publish, translate, create derivative works from, distribute, >>> perform, display and use Your Submissions (in whole or part) and to >>> incorporate or implement them in other works in any form, media, or >>> technology now known or later developed. This includes, without limitation, >>> the right to incorporate or implement the Submission into any product or >>> service, and to display, market, sublicense and distribute the Submissions >>> as incorporated or embedded in any product or service distributed or >>> offered by Oracle without compensation to you. >> >> (Might be good to rework that to actually encourage contributions. I am kind >> of surprised that any company is willing to contribute to the OpenJDK under >> these terms.) >> >> Sorry for driving this pull request off-topic, I will stop all interaction >> here and will not bother you any further here if that is desired. Below is >> the original comment: >> >> ----- >> >> It might also be good to clarify in the documentation that `parseHex(...)` >> is case insensitive. This is currently not obvious and one might expect that >> it respects `isUpperCase()`. >> >> Additionally there are quite a lot places within the JDK code where this new >> class could be used. I have picked some where it would definitely be an >> improvement (but omitted test classes): >> <details> >> <summary>Classes</summary> >> <ul> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/java/net/HostPortrange.java#L94";><code>java.base</code> >> <code>java.net.HostPortrange</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/jdk/internal/module/ModuleHashes.java#L215";><code>java.base</code> >> <code>jdk.internal.module.ModuleHashes</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java#L544";><code>java.base</code> >> <code>sun.net.www.protocol.http.DigestAuthentication</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java#L738";><code>java.base</code> >> <code>sun.security.provider.AbstractDrbg</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/provider/certpath/ResponderId.java#L269";><code>java.base</code> >> <code>sun.security.provider.certpath.ResponderId</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/ssl/Utilities.java#L39";><code>java.base</code> >> <code>sun.security.ssl.Utilities</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/util/Debug.java#L323";><code>java.base</code> >> <code>sun.security.util.Debug</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/util/HexDumpEncoder.java";><code>java.base</code> >> <code>sun.security.util.HexDumpEncoder</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java#L244";><code>java.base</code> >> <code>sun.security.util.ManifestEntryVerifier</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java#L753";><code>java.base</code> >> <code>sun.security.util.SignatureFileVerifier</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/x509/AVA.java#L742";><code>java.base</code> >> <code>sun.security.x509.AVA</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/x509/AVA.java#L811";><code>java.base</code> >> <code>sun.security.x509.AVA</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/x509/AVA.java#L889";><code>java.base</code> >> <code>sun.security.x509.AVA</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/x509/AVA.java#L967";><code>java.base</code> >> <code>sun.security.x509.AVA</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/x509/AVA.java#L1111";><code>java.base</code> >> <code>sun.security.x509.AVA</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.base/share/classes/sun/security/x509/X509CertImpl.java#L1982";><code>java.base</code> >> <code>sun.security.x509.X509CertImpl</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.naming/share/classes/com/sun/jndi/ldap/LdapName.java#L837";><code>java.naming</code> >> <code>com.sun.jndi.ldap.LdapName</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.naming/share/classes/javax/naming/ldap/Rdn.java#L568";><code>java.naming</code> >> <code>javax.naming.ldap.Rdn</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.security.jgss/share/classes/sun/security/krb5/KrbApReq.java#L314";><code>java.security.jgss</code> >> <code>sun.security.krb5.KrbApReq</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.security.jgss/share/classes/sun/security/krb5/internal/ktab/KeyTabEntry.java#L80";><code>java.security.jgss</code> >> <code>sun.security.krb5.internal.ktab.KeyTabEntry</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/java.smartcardio/share/classes/sun/security/smartcardio/PCSC.java#L174";><code>java.smartcardio</code> >> <code>sun.security.smartcardio.PCSC</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.compiler/share/classes/com/sun/tools/javac/file/BaseFileManager.java#L355";><code>jdk.compiler</code> >> <code>com.sun.tools.javac.file.BaseFileManager</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.compiler/share/classes/com/sun/tools/javac/main/Main.java#L490";><code>jdk.compiler</code> >> <code>com.sun.tools.javac.main.Main</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Util.java#L158";><code>jdk.crypto.cryptoki</code> >> <code>sun.security.pkcs11.P11Util</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/Functions.java#L168";><code>jdk.crypto.cryptoki</code> >> <code>sun.security.pkcs11.wrapper.Functions</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.java/src/org/graalvm/compiler/java/LambdaUtils.java#L129";><code>jdk.internal.vm.compiler</code> >> <code>org.graalvm.compiler</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jartool/share/classes/sun/tools/jar/Main.java#L2011";><code>jdk.jartool</code> >> <code>sun.tools.jar.Main</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jconsole/share/classes/sun/tools/jconsole/AboutDialog.java#L83";><code>jdk.jconsole</code> >> <code>sun.tools.jconsole.AboutDialog</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jconsole/share/classes/sun/tools/jconsole/ConnectDialog.java#L287";><code>jdk.jconsole</code> >> <code>sun.tools.jconsole.ConnectDialog</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jconsole/share/classes/sun/tools/jconsole/ConnectDialog.java#L595";><code>jdk.jconsole</code> >> <code>sun.tools.jconsole.ConnectDialog</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jconsole/share/classes/sun/tools/jconsole/HTMLPane.java#L61";><code>jdk.jconsole</code> >> <code>sun.tools.jconsole.HTMLPane</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jconsole/share/classes/sun/tools/jconsole/inspector/XArrayDataViewer.java#L79";><code>jdk.jconsole</code> >> <code>sun.tools.jconsole.inspector.XArrayDataViewer</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jdeps/share/classes/com/sun/tools/javap/AttributeWriter.java#L1137";><code>jdk.jdeps</code> >> <code>com.sun.tools.javap.AttributeWriter</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jdeps/share/classes/com/sun/tools/javap/ClassWriter.java#L149";><code>jdk.jdeps</code> >> <code>com.sun.tools.javap.ClassWriter</code></li> >> <li><a >> href="https://github.com/openjdk/jdk/blob/83ea863122e1d58527c8bf3cd178acf4d0c54f30/src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java#L417";><code>jdk.jlink</code> >> <code>jdk.tools.jmod.JmodTask</code></li> >> </ul> > > Hi, > > The adoption of the HexFormat API by other JDK classes is a separate task. > In the security area, a pre-Git webrev included the changes, when the > API settles > that will come back to the fore. In many of the security tests, it may > be better to > utilize the test library addition that can format ASN.1/DER streams to > make the output easier to read. > > The point about case insensitive parsing can be reinforced. > > Regards, Roger The HexFormat javadoc is updated to include the `fromIndex`/`toIndex' API changes. http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html The previous `index`/`length' indexed API is retained for comparison. http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter-index-length/docs/api/java.base/java/util/HexFormat.html ------------- PR: https://git.openjdk.java.net/jdk/pull/482
