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/~vinnie/8170769/webrev.08/>
http://cr.openjdk.java.net/~vinnie/8170769/javadoc.08/api/java.base/java/util/HexFormat.html
<http://cr.openjdk.java.net/~vinnie/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.ri...@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
