RFR: 8170769 Provide a simple hexdump facility for binary data

[Roger Riggs]

Hi,

Looks good, though some points to clarify.

- The methods that use ByteBuffers should be clear that accesses to the 
ByteBuffers
    are relative and use and modify the position and ByteBuffer 
exceptions may be thrown.

- The methods that write output (Strings) to an output stream might be 
more general
   if the argument was a PrintStream, allowing the hex formatted output 
to be
   assembled with other localized output.
   I do see that as long as HexFormat is only writing hex digits, the 
effect would be almost the same.
   (It would also eliminate, the inefficient code in getPrintStream that 
creates a PrintStream on demand).

  - toString(byte[], from, to) and other methods there's no need for 
parens around the note about fromIndex==toIndex.

  - fromString methods: The optional prefix of "0x": add the phrase "is 
ignored".
     The IAE, does not mention the optional prefix, I'm not sure if that 
allows some confusion.

- dumpAsStream(InputStream) does not have a throws NPE for in == null.
   A catch all in the class javadoc could cover that.
   " Unless otherwise noted, passing a null argument to a constructor or 
method in any class or
    interface in this package will cause a |NullPointerException| 
<https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/NullPointerException.html> 
to be thrown. "

  - dumpAsStream methods, the formatter argument is described as being 
used "if present";
    it might be clearer as "if not null".

  - dumpAsStream(ByteBuffer, from, to, chunk, Formatter) may be 
confusing because it
    is using the relative methods of ByteBuffer, but the from and to 
offsets are within
    the position .. limit buffer.  That should be explicit.
    (Or consider switching to the absolute accesses in the ByteBuffer 
and not affect the position)

- dump(byte[], OutputStream) - I don't think the example is correct, 
there is no reference
   to a stream, only the PrintStream::println method, which is not static.

Regards, Roger



On 12/07/2018 08:18 PM, Vincent Ryan wrote:
> Here’s the latest version that addresses all the current open issues:
>
> webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.07/ 
> <http://cr.openjdk.java.net/%7Evinnie/8170769/webrev.07/>
>
> javadoc: 
> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html 
> <http://cr.openjdk.java.net/%7Evinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html>
>
>
> CSR: https://bugs.openjdk.java.net/browse/CCC-8170769
>
>
>> On 7 Dec 2018, at 19:51, Vincent Ryan <vincent.x.ryan at oracle.com 
>> <mailto:vincent.x.ryan at oracle.com>> wrote:
>>
>> Even shorter. I’ll add that instead.
>> Thanks.
>>
>>> On 7 Dec 2018, at 19:04, Roger Riggs <Roger.Riggs at oracle.com 
>>> <mailto:Roger.Riggs at oracle.com>> wrote:
>>>
>>> Hi,
>>>
>>> I don't think this is performance sensitive and less code is better.
>>>
>>> Use java.lang.Character.digit(ch, 16) to convert the char to an int.
>>>
>>> Roger
>>>
>>> On 12/07/2018 01:49 PM, Kasper Nielsen wrote:
>>>> Hi,
>>>>
>>>> I don't know if performance is an issue. But if it is, I think it world
>>>> make sense to use a precompute array. And replace
>>>>
>>>> private static int hexToBinary(char ch) {
>>>>    if ('0' <= ch && ch <= '9') {
>>>>        return ch - '0';
>>>>    }
>>>>    if ('A' <= ch && ch <= 'F') {
>>>>        return ch - 'A' + 10;
>>>>    }
>>>>    if ('a' <= ch && ch <= 'f') {
>>>>        return ch - 'a' + 10;
>>>>    }
>>>>    return -1;
>>>> }
>>>>
>>>> with
>>>>
>>>> private static int hexToBinary(char ch) {
>>>>    return ch <= 'f' ? staticPrecomputedArray[ch] : -1;
>>>> }
>>>>
>>>> where int[] staticPrecomputedArray is computed in a static initializer
>>>> block.
>>>>
>>>> /Kasper
>>>>
>>>> On Fri, 23 Nov 2018 at 14:51, Vincent Ryan 
>>>> <vincent.x.ryan at oracle.com <mailto:vincent.x.ryan at oracle.com>>
>>>> wrote:
>>>>
>>>>> Hello,
>>>>>
>>>>> Please review this proposal for a new API to conveniently generate and
>>>>> display binary data using hexadecimal string representation.
>>>>> It supports both bulk and stream operations and it can also 
>>>>> generate the
>>>>> well-known hexdump format [1].
>>>>>
>>>>> This latest revision addresses review comments provided earlier.
>>>>> These include adding methods to support for data supplied in a
>>>>> ByteBuffer, exposing the default formatter implementation as a public
>>>>> static and dropping the superfluous 'Hex' term from several method 
>>>>> names.
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170769
>>>>> API:
>>>>>
>>>>> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html 
>>>>> <http://cr.openjdk.java.net/%7Evinnie/8170769/javadoc.06/api/java.base/java/util/Hex.html>
>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/
>>>>>
>>>>>
>>>>> ____
>>>>> [1] https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html
>>>>>
>>>
>>
>