Hi Patrick,
A few comments:
Readable.java:
67: + it may be worth mentioning that the input might not fit in
output (as seen in the CharBuffer case)
Though I see we didn't call that out in the other transferTo description
but here it is more likely that the output is bounded.
77: "The total amount will be added up by after the write method has
been completed." should not be part
of the @implSpec. It is untestable and unnecessary. If an exception
occurs the value is lost.
96: "in order not having the extra overhead creating" -> "to avoid the
extra overhead of"
X-Buffer.java.template:
1555: "form" -> "from"
1554: I would avoid most of the details in the @implSpec since it is
requiring a specific use of CharBuffer methods.
That's fine as an @ImplNote but restricts the implementation too much as
spec.
(others will disagree)
1565: in the code, perhaps it should use an explicit
RequireNonNull(out, "out") otherwise the NPE will be on put()/append().
1566: "insufficient space in this" refers to the source, not dest and it
should only apply if out is a CharBuffer.
Omit it or leave it more general; that behavior is covered by the
spec of the out Appendable.
1568: I don't see code to enforce: "if out is this buffer"
On the tests; had you considered using TestNG;
it provides some good structure and is preferred (AFAIK) for new tests.
As for Randomness, it is useful to be explicit about the seed used in
the particular run so it can be
replayed if necessary. There is a testlibrary function to do that if
you don't want to code-your-own.
(test/jdk/test/lib/RandomFactory::getRandom())
Thanks, Roger
On 11/9/2017 10:37 AM, Patrick Reinhart wrote:
Finally did some review with Alan and integrated all into this webrev:
http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.01
<http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.01>
-Patrick
Am 08.11.2017 um 00:03 schrieb Brian Burkhalter <brian.burkhal...@oracle.com>:
Hi Patrick,
On Nov 7, 2017, at 3:01 PM, Patrick Reinhart <patr...@reini.net
<mailto:patr...@reini.net>> wrote:
I integrated your changes into my current version which I will look into
tomorrow with Alan at Devoxx. I think adding some implementation note on the
default method implementation will be needed to review later on…
I concur.
I also made an override on the CharBuffer, where we need to add a enhanced
documentation for the specialties around the CharBuffer implementations
(BufferOverflowException/ReadOnlyBufferException), where a review of some
native english speakers would help :-)
Sounds good!
Thanks,
Brian
