RFR: 8170769 Provide a simple hexdump facility for binary data

[Roger Riggs]

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.

>
>
>>
>> - 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.

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/ 
>>> <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
>>>>>>>
>>>>>
>>>>
>>>
>>
>