Responses in-line.
> On 10 Dec 2018, at 21:38, Roger Riggs <roger.ri...@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.ri...@oracle.com
>>> <mailto:roger.ri...@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.r...@oracle.com> wrote:
>>>>>
>>>>> Even shorter. I’ll add that instead.
>>>>> Thanks.
>>>>>
>>>>>> On 7 Dec 2018, at 19:04, Roger Riggs <roger.ri...@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.r...@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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
