RFR: 8251989: Hex formatting and parsing utility [v8]

Tue Oct 27 03:09:32 UTC 2020

On Fri, 23 Oct 2020 16:34:54 GMT, Roger Riggs <rriggs at 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