RFR: 8170769 Provide a simple hexdump facility for binary data

Vincent Ryan vincent.x.ryan at oracle.com
Tue Dec 11 16:34:32 UTC 2018 

Responses in-line.

> On 10 Dec 2018, at 21:38, Roger Riggs <Roger.Riggs at oracle.com> wrote:
> 
> Hi Vincent,
> 
> On 12/10/2018 03:59 PM, Vincent Ryan wrote:
>> Comments in-line.
>> Thanks.
>> 
>>> On 10 Dec 2018, at 16:59, Roger Riggs <Roger.Riggs at oracle.com <mailto:Roger.Riggs at oracle.com>> wrote:
>>> 
>>> 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.
>> 
>> Added the line:
>> 'Access to the ByteBuffer is relative and its position gets set to the buffer's limit.'
> ok
>> 
>>> 
>>> - 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).
>> 
>> I had wanted to provide flexibility by favouring OutputStream over PrintStream.
>> It is convenient for the common case of dumping to a file using 'new FileOutputStream’.
>> As you note it complicates assembly with other output.
>> 
>> I guess it’s fine to change the OutputStream args to PrintStream.
> ok, if the caller has a direct OutputStream, a PrintStream could be created.
> But I think the PrintStream is more common.
>> 
>> 
>>> 
>>>  - toString(byte[], from, to) and other methods there's no need for parens around the note about fromIndex==toIndex.
>> 
>> OK
>> 
>>> 
>>>  - 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.
>> 
>> Added the line:
>> 'The optional prefix of {@code "0x"} is ignored.'
> ok
>> 
>>> 
>>> - 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 to be thrown. “
>> 
>> Added an @throws to the method.
> ok
>> 
>>> 
>>>  - dumpAsStream methods, the formatter argument is described as being used "if present";  
>>>    it might be clearer as "if not null”.
>> 
>> OK
>> Its really nice/necessary that examples can be copy/pasted and compile.
>>> 
>>> 
>>>  - 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)
>> 
>> Is the additional javadoc line added earlier sufficient?
> I would bear some reinforcing that the entire remainder of the buffer is read
> and the from and to are indexes within that remainder.
> And I'm not sure that's the desirable semantics.
> 
> It would make more sense if the from bytes from the buffer are skipped
> and only the (to - from) bytes are dumped.  That leaves the ByteBuffer 
> reading only the requested bytes.  A natural usage such as:
>  dumpAsStream​(ByteBuffer buffer, 0, 256, 16, formatter)
> would dump the next 256bytes of the ByteBuffer and position would be
> moved by 256.

[As suggested by Alan]
How about dropping the fromIndex and toIndex parameters and requiring the caller
to provide a suitably sized buffer? 

public static Stream<String> dumpAsStream(ByteBuffer buffer, int chunkSize, Formatter formatter) {


> 
>> 
>> 
>>> 
>>> - 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.
>> 
>> The code is just illustrative. Real values would have to be supplied for the input bytes and the
>> chosen print method on the output stream. Typically, that print method will be System.out::println
> Examples that don't compile are really confusing and annoying.

Updated the ‘as if’ code to:

     *     byte[] bytes = ...
     *     PrintStream out = ...
     *     HexFormat.dumpAsStream(bytes, 16,
     *         (offset, chunk, from, to) ->
     *             String.format("%08x  %s  |%s|",
     *                 offset,
     *                 HexFormat.toFormattedString(chunk, from, to),
     *                 HexFormat.toPrintableString(chunk, from, to)))
     *         .forEachOrdered(out::println);


> 
> Thanks, Roger
> 
>> 
>> 
>>> 
>>> 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/
>>>> javadoc: http://cr.openjdk.java.net/~vinnie/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> wrote:
>>>>> 
>>>>> Even shorter. I’ll add that instead.
>>>>> Thanks.
>>>>> 
>>>>>> On 7 Dec 2018, at 19:04, Roger Riggs <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>
>>>>>>> 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
>>>>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8170769/webrev.06/
>>>>>>>> 
>>>>>>>> 
>>>>>>>> ____
>>>>>>>> [1] https://docs.oracle.com/cd/E88353_01/html/E37839/hexdump-1.html
>>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>

More information about the core-libs-dev mailing list