JDK-8067661: transferTo proposal for Appendable
Roger.Riggs at Oracle.com
Thu Nov 9 20:19:40 UTC 2017
A few comments:
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"
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
(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.
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>
>> Am 08.11.2017 um 00:03 schrieb Brian Burkhalter <brian.burkhalter at oracle.com>:
>> Hi Patrick,
>> On Nov 7, 2017, at 3:01 PM, Patrick Reinhart <patrick at reini.net <mailto:patrick at 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!
More information about the core-libs-dev