Thanks for your review Alan. I believe I’ve addressed your concerns 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>
> On 11 Dec 2018, at 13:04, Alan Bateman <alan.bate...@oracle.com> wrote: > > On 08/12/2018 01:18, 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/~vinnie/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/~vinnie/8170769/javadoc.07/api/java.base/java/util/HexFormat.html> >> >> CSR: https://bugs.openjdk.java.net/browse/CCC-8170769 >> <https://bugs.openjdk.java.net/browse/CCC-8170769> >> > I read through the latest javadoc corresponding to webrev.07. > > Overall I think it looks very good except for dumpAsStream(ByteBuffer, > fromIndex, toIndex, chunkSize, Formatter) as it's not clear if fromIndex is > from the buffer position or an absolute index. If the former then there are a > couple of other issues. I see Roger has touched on the advancement of the > buffer position to its limit which isn't right unless all remaining bytes in > the buffer are consumer. There is also an issue with the exception as > attempting to consume beyond the limit is a BufferUnderflowException. It > might be simpler to replace this method with a dumpAsStream(ByteBuffer, > chunkSize, Formatter) that can lazily (rather than eagerly) consume the > remaining bytes. Additional overloads could be added in the future if needed. > > dumpAsStream(InputStream) specifies "On return, the input stream will be at > end-of-stream" but I assume this isn't right as the method is lazy. > > The 3-arg dumpAsStream doesn't specify the exception/behavior for when > chunkSize is <= 0. > > fromString(CharSequence) may need clarification on how it behaves when the CS > is a CharBuffer. Does it consume all the remaining chars in the buffer so its > position be advanced to the limit? The 3-arg version is also not clear on > this point. > > In Formatter interface it may be useful to expand the description of the > "offset" parameter as readers may not immediately understand that it's just > an input for the formatted string rather than a real offset, an example might > help. It also doesn't say if the offset can be specified as <= 0L or not. > > A minor comment is that several places refer to a byte array as a "byte > buffers. Is this deliberate or can these be replaced with "byte array" to > avoid confusion. Another minor comment is that one of the dumpAsStream > methods is missing @throws NPE - maybe they should all be removed and > replaced with a statement in the class description. > > -Alan
