RFR: 8170769 Provide a simple hexdump facility for binary data

[Roger Riggs]

Hi Vincent,

I have no new inclinations about relative vs absolute, maybe someone 
else has
use cases that will tip the balance.

HexFormat.java:

  - line 264: I think you can safely just append the (char) (byte[i] & 
0xff). (to avoid sign extension).
    Creating a string for each char seems like a waste.

- 759:  The getPrintStream(out) method can be removed since its always a 
PrintStream

- 733: The exception containing the entire input hex string might be a 
bit daunting.
     Can it return just the bad character and perhaps the offset?

- The 'infinite' in the @return of two of the dumpAsStream methods could 
be removed.
   The bound on the stream depends on the input and its not important to 
say its infinite.

Otherwise, looking very good.

Thanks, Roger




On 12/11/2018 04:21 PM, Vincent Ryan wrote:
> Thanks Roger.
>
> I believe that all but one of your concerns are addressed in this 
> latest webrev/javadoc:
> http://cr.openjdk.java.net/~vinnie/8170769/webrev.08/ 
> <http://cr.openjdk.java.net/%7Evinnie/8170769/webrev.08/>
> http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html 
> <http://cr.openjdk.java.net/%7Evinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html>
>
> The remaining issue is how best to define the dumpAsStream() method 
> that takes a ByteBuffer.
> The current approach has dropped the to/from parameters, consumes all 
> the buffer and advances
> Its position to the buffer limit.
> This places the burden on the caller to size the buffer appropriately.
>
> However I also see the merit in a ‘read-only’ approach that does not 
> advance the buffer position.
>
> Any further thoughts?
>
>
>
>> On 11 Dec 2018, at 16:54, Roger Riggs <Roger.Riggs at oracle.com 
>> <mailto:Roger.Riggs at oracle.com>> wrote:
>>
>> Ho Vincent,
>>
>> On 12/11/2018 11:34 AM, Vincent Ryan wrote:
>>> Responses in-line.
>>>
>>>>
>>>>> 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) {
>>>
>> I'd go the other direction and make dump use absolute offset and limit.
>> The current values for position and limit are readily available from 
>> the buffer.
>>
>> If the dumping code is being added to perform some diagnostics then 
>> it would not
>> modify the position and not disturb the existing code that is using 
>> the buffer.
>>
>> Absolute is simpler to explain and implement and has fewer side effects.
>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> - 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);
>>>
>>>
>> Looks fine, thanks
>