JDK-8067661: transferTo proposal for Appendable

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
Hi Chris & Pavel,

Based on the transferTo Method on the InputStream I would propose to introduce a default method on the Readable interface. In that way the method can be used for all existing implementations of Readable and still be implemented in a special way by a individual implementer.

    /**
     * Reads all characters from this readable and writes the characters to
     * the given appendable in the order that they are read. On return, this
     * readable will be at end its data.
     * <p>
     * This method may block indefinitely reading from the readable, or
     * writing to the appendable. The behavior for the case where the readable
     * and/or appendable is <i>asynchronously closed</i>, or the thread
     * interrupted during the transfer, is highly readable and appendable
     * specific, and therefore not specified.
     * <p>
     * If an I/O error occurs reading from the readable or writing to the
     * appendable, then it may do so after some characters have been read or
     * written. Consequently the readable may not be at end of its data and
     * one, or both participants may be in an inconsistent state. It is strongly
     * recommended that both readable and appendable be promptly closed
     * if needed and an I/O error occurs.
     *
     * @param  out the appendable, non-null
     * @return the number of characters transferred
     * @throws IOException if an I/O error occurs when reading or writing
     * @throws NullPointerException if {@code out} is {@code null}
     *
     * @since 1.9
     */
    default long transferTo(Appendable out) throws IOException {
    ....
    }


-Patrick


Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Pavel Rappo
Hi Patrick, nice to hear from you again! I agree we should consider adding this
method. Unfortunately, from the spec point of view I suppose this one will
require a lot more chewing. For instance there's nothing that can be closed
either in Readable or Appendable (in general case), since neither of them is
java.io.Closeable or even java.lang.AutoCloseable. In my opinion, all mentions
of 'close' operations should go. You're fine to elaborate spec down the
hierarchy though:

    /**
     *
     ... some specifics for Readers ...
     *
     */
    @Override
    public long transferTo(Appendable out) throws IOException {
        return Readable.super.transferTo(out);
    }

But at the same time this one, you added, is crucial:

    * one, or both participants may be in an inconsistent state
   
So you give implementors the freedom to use "if anything bad happens all bets
are off".
It's also not fully clear what it means for an instance of Readable to be
"at end of its data". This have a well-defined meaning for java.io.Reader though.

-Pavel

> On 16 Dec 2014, at 22:13, Patrick Reinhart <[hidden email]> wrote:
>
> Hi Chris & Pavel,
>
> Based on the transferTo Method on the InputStream I would propose to introduce a default method on the Readable interface. In that way the method can be used for all existing implementations of Readable and still be implemented in a special way by a individual implementer.
>
>    /**
>     * Reads all characters from this readable and writes the characters to
>     * the given appendable in the order that they are read. On return, this
>     * readable will be at end its data.
>     * <p>
>     * This method may block indefinitely reading from the readable, or
>     * writing to the appendable. The behavior for the case where the readable
>     * and/or appendable is <i>asynchronously closed</i>, or the thread
>     * interrupted during the transfer, is highly readable and appendable
>     * specific, and therefore not specified.
>     * <p>
>     * If an I/O error occurs reading from the readable or writing to the
>     * appendable, then it may do so after some characters have been read or
>     * written. Consequently the readable may not be at end of its data and
>     * one, or both participants may be in an inconsistent state. It is strongly
>     * recommended that both readable and appendable be promptly closed
>     * if needed and an I/O error occurs.
>     *
>     * @param  out the appendable, non-null
>     * @return the number of characters transferred
>     * @throws IOException if an I/O error occurs when reading or writing
>     * @throws NullPointerException if {@code out} is {@code null}
>     *
>     * @since 1.9
>     */
>    default long transferTo(Appendable out) throws IOException {
>    ....
>    }
>
>
> -Patrick
>
>

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
Hi Pavel,

> Hi Patrick, nice to hear from you again! I agree we should consider adding this
> method. Unfortunately, from the spec point of view I suppose this one will
> require a lot more chewing. For instance there's nothing that can be closed
> either in Readable or Appendable (in general case), since neither of them is
> java.io.Closeable or even java.lang.AutoCloseable. In my opinion, all mentions
> of 'close' operations should go. You're fine to elaborate spec down the
> hierarchy though:
>
>    /**
>     *
>     ... some specifics for Readers ...
>     *
>     */
>    @Override
>    public long transferTo(Appendable out) throws IOException {
>        return Readable.super.transferTo(out);
>    }

You’re right. I also saw that neither Readable nor Appendable extend any of those *Closable interfaces.
I was not sure if that should belong to the Readable itself. To move those more specific parts down to
the Reader implementation is absolutely fine for me and seems to be a better fit.

>
> But at the same time this one, you added, is crucial:
>
>    * one, or both participants may be in an inconsistent state
>
> So you give implementors the freedom to use "if anything bad happens all bets
> are off".
> It's also not fully clear what it means for an instance of Readable to be
> "at end of its data". This have a well-defined meaning for java.io.Reader though.
>

I got your point here. I was not sure, what parts from the InputStream.transferTo() spec can be reused.

-Patrick

>
>> On 16 Dec 2014, at 22:13, Patrick Reinhart <[hidden email]> wrote:
>>
>> Hi Chris & Pavel,
>>
>> Based on the transferTo Method on the InputStream I would propose to introduce a default method on the Readable interface. In that way the method can be used for all existing implementations of Readable and still be implemented in a special way by a individual implementer.
>>
>>   /**
>>    * Reads all characters from this readable and writes the characters to
>>    * the given appendable in the order that they are read. On return, this
>>    * readable will be at end its data.
>>    * <p>
>>    * This method may block indefinitely reading from the readable, or
>>    * writing to the appendable. The behavior for the case where the readable
>>    * and/or appendable is <i>asynchronously closed</i>, or the thread
>>    * interrupted during the transfer, is highly readable and appendable
>>    * specific, and therefore not specified.
>>    * <p>
>>    * If an I/O error occurs reading from the readable or writing to the
>>    * appendable, then it may do so after some characters have been read or
>>    * written. Consequently the readable may not be at end of its data and
>>    * one, or both participants may be in an inconsistent state. It is strongly
>>    * recommended that both readable and appendable be promptly closed
>>    * if needed and an I/O error occurs.
>>    *
>>    * @param  out the appendable, non-null
>>    * @return the number of characters transferred
>>    * @throws IOException if an I/O error occurs when reading or writing
>>    * @throws NullPointerException if {@code out} is {@code null}
>>    *
>>    * @since 1.9
>>    */
>>   default long transferTo(Appendable out) throws IOException {
>>   ....
>>   }
>>
>>
>> -Patrick
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Alan Bateman
In reply to this post by Pavel Rappo
On 16/12/2014 22:54, Pavel Rappo wrote:
> Hi Patrick, nice to hear from you again! I agree we should consider adding this
> method. Unfortunately, from the spec point of view I suppose this one will
> require a lot more chewing. For instance there's nothing that can be closed
> either in Readable or Appendable (in general case), since neither of them is
> java.io.Closeable or even java.lang.AutoCloseable. In my opinion, all mentions
> of 'close' operations should go.
I agree that transferTo(Writer) is a no-brainer. When changed to be more
general and Appendable then it might be simplest to just deal with the
AutoCloseable case in the javadoc.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
I tried to put that in my latest proposal:

/**
 * Reads all characters from this readable and writes the characters to
 * the given appendable in the order that they are read. On return, this
 * readable will be at end its data.
 * <p>
 * This method may block indefinitely reading from the readable, or
 * writing to the appendable. The behavior for the case where the readable
 * and/or appendable is <i>asynchronously closed</i>, or the thread
 * interrupted during the transfer, is highly readable and appendable
 * specific, and therefore not specified.
 * <p>
 * If an I/O error occurs reading from the readable or writing to the
 * appendable, then it may do so after some characters have been read or
 * written. Consequently the readable may not be at end of its data and
 * one, or both participants may be in an inconsistent state. That in mind
 * all additional measures required by one or both participants in order to
 * eventually free their internal resources have to be taken by the caller
 * of this method.
 *
 * @param  out the appendable, non-null
 * @return the number of characters transferred
 * @throws IOException if an I/O error occurs when reading or writing
 * @throws NullPointerException if {@code out} is {@code null}
 *
 * @since 18.3
 */
default long transferTo(Appendable out) throws IOException {
....
}

-Patrick

Am 01.11.2017 um 13:42 schrieb Alan Bateman:

> On 16/12/2014 22:54, Pavel Rappo wrote:
>> Hi Patrick, nice to hear from you again! I agree we should consider
>> adding this
>> method. Unfortunately, from the spec point of view I suppose this one
>> will
>> require a lot more chewing. For instance there's nothing that can be
>> closed
>> either in Readable or Appendable (in general case), since neither of
>> them is
>> java.io.Closeable or even java.lang.AutoCloseable. In my opinion, all
>> mentions
>> of 'close' operations should go.
> I agree that transferTo(Writer) is a no-brainer. When changed to be
> more general and Appendable then it might be simplest to just deal
> with the AutoCloseable case in the javadoc.
>
> -Alan



Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Alan Bateman
On 01/11/2017 14:57, Patrick Reinhart wrote:
> I tried to put that in my latest proposal:
>
> /**
>   * Reads all characters from this readable and writes the characters to
>   * the given appendable in the order that they are read. On return, this
>   * readable will be at end its data.
You might want to try using  "this source" in the wording, as in "On
return, the source of characters will be at its end". I think this will
make it a bit more consistent with the existing wording in the read method.

>   * <p>
>   * This method may block indefinitely reading from the readable, or
>   * writing to the appendable. The behavior for the case where the readable
>   * and/or appendable is <i>asynchronously closed</i>, or the thread
>   * interrupted during the transfer, is highly readable and appendable
>   * specific, and therefore not specified.
I think this needs more setup to allow for close, maybe "Where this
source or the appendable is {@link AutoCloseable closeable} then the
behavior when either are asynchronously closed ...".


>   *
>   * @since 18.3
>
We're using "@since 10" for now, pending an outcome of the version
string discussion.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
Hi Alan,

The latest version is here:

http://cr.openjdk.java.net/~reinhapa/reviews/8067661/signature


> Am 02.11.2017 um 14:52 schrieb Alan Bateman <[hidden email]>:
>
> On 01/11/2017 14:57, Patrick Reinhart wrote:
>> I tried to put that in my latest proposal:
>>
>> /**
>>  * Reads all characters from this readable and writes the characters to
>>  * the given appendable in the order that they are read. On return, this
>>  * readable will be at end its data.
> You might want to try using  "this source" in the wording, as in "On return, the source of characters will be at its end". I think this will make it a bit more consistent with the existing wording in the read method.

That would read then something like this:

 * Reads all characters from this readable and writes the characters to
 * the given appendable in the order that they are read. On return, the
 * source of characters will be at its end.


>
>>  * <p>
>>  * This method may block indefinitely reading from the readable, or
>>  * writing to the appendable. The behavior for the case where the readable
>>  * and/or appendable is <i>asynchronously closed</i>, or the thread
>>  * interrupted during the transfer, is highly readable and appendable
>>  * specific, and therefore not specified.
> I think this needs more setup to allow for close, maybe „Where this source or the appendable is {@link AutoCloseable closeable} then the behavior when either are asynchronously closed …“.

The second part:

 * This method may block indefinitely reading from the readable, or
 * writing to the appendable. Where this source or the appendable is
 * {@link java.io.AutoCloseable closeable}, then the behavior when either are
 * <i>asynchronously closed</i>, or the thread interrupted during the transfer,
 * is highly readable and appendable specific, and therefore not specified.

The only thing that worries me is that the @link is pointing form java.lang to java.io stuff, which I tried to prevent.

>
>>  *
>>  * @since 18.3
>>
> We're using "@since 10" for now, pending an outcome of the version string discussion.
>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Alan Bateman
On 02/11/2017 18:36, Patrick Reinhart wrote:
> :
> That would read then something like this:
>
>   * Reads all characters from this readable and writes the characters to
>   * the given appendable in the order that they are read. On return, the
>   * source of characters will be at its end.
Yes, or even "Reads all characters from this source ...".


> :
> The second part:
>
>   * This method may block indefinitely reading from the readable, or
>   * writing to the appendable. Where this source or the appendable is
>   * {@link java.io.AutoCloseable closeable}, then the behavior when either are
>   * <i>asynchronously closed</i>, or the thread interrupted during the transfer,
>   * is highly readable and appendable specific, and therefore not specified.
>
> The only thing that worries me is that the @link is pointing form java.lang to java.io stuff, which I tried to prevent.
>
The read method reads from a CharBuffer and throws IOException so I
think this is okay.

So are you planning to adding an implementation + tests for this?

-Alan


Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
Hi Alan,

> Am 02.11.2017 um 20:34 schrieb Alan Bateman <[hidden email]>:
>
> On 02/11/2017 18:36, Patrick Reinhart wrote:
>> :
>> That would read then something like this:
>>
>>  * Reads all characters from this readable and writes the characters to
>>  * the given appendable in the order that they are read. On return, the
>>  * source of characters will be at its end.
> Yes, or even „Reads all characters from this source …“

Updated accordingly:

http://cr.openjdk.java.net/~reinhapa/reviews/8067661/signature

>
>> :
>> The second part:
>>
>>  * This method may block indefinitely reading from the readable, or
>>  * writing to the appendable. Where this source or the appendable is
>>  * {@link java.io.AutoCloseable closeable}, then the behavior when either are
>>  * <i>asynchronously closed</i>, or the thread interrupted during the transfer,
>>  * is highly readable and appendable specific, and therefore not specified.
>>
>> The only thing that worries me is that the @link is pointing form java.lang to java.io stuff, which I tried to prevent.
>>
> The read method reads from a CharBuffer and throws IOException so I think this is okay.
>
> So are you planning to adding an implementation + tests for this?
>

If we are all happy with the API so far, I could start adding an initial implementation and test…

-Patrick
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Alan Bateman
On 02/11/2017 19:55, Patrick Reinhart wrote:
> :
> If we are all happy with the API so far, I could start adding an initial implementation and test…
>
I think the API looks fine so go ahead.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
Hi Alan,

I will have a version ready at Devoxx next week ;-)

-Patrick


> Am 03.11.2017 um 13:49 schrieb Alan Bateman <[hidden email]>:
>
> On 02/11/2017 19:55, Patrick Reinhart wrote:
>> :
>> If we are all happy with the API so far, I could start adding an initial implementation and test…
>>
> I think the API looks fine so go ahead.
>
> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Brian Burkhalter-2
In reply to this post by Alan Bateman
I am a little late to this thread, but some alternative verbiage to consider is included below. This wording however diverges from that of [1] so if we want them to remain similar then it’s probably not worth it to change from what has been agreed upon already.

Thanks,

Brian

[1] https://docs.oracle.com/javase/9/docs/api/java/io/InputStream.html#transferTo-java.io.OutputStream-

  57      * Reads all characters from this source and writes the characters to
  58      * the given appendable in the order that they are read. On return, the
  59      * source of characters will be at its end.

Reads all characters from this source and appends them to a destination in
the order in which they are read. On return, ...

  60      * <p>
  61      * This method may block indefinitely reading from the readable, or
  62      * writing to the appendable. Where this source or the appendable is
  63      * {@link AutoCloseable closeable}, then the behavior when either are
  64      * <i>asynchronously closed</i>, or the thread interrupted during the transfer,
  65      * is highly readable and appendable specific, and therefore not specified.

This method may block indefinitely while reading from the source or writing to the destination.
If the source or destination is {@link AutoCloseable closeable}, then the behavior when either
is <i>asynchronously closed</i>, or the thread is interrupted during the transfer, is highly
implementation-dependent and hence unspecified.

  66      * <p>
  67      * If an I/O error occurs reading from the readable or writing to the
  68      * appendable, then it may do so after some characters have been read or
  69      * written. Consequently the readable may not be at end of its data and
  70      * one, or both participants may be in an inconsistent state. That in mind
  71      * all additional measures required by one or both participants in order to
  72      * eventually free their internal resources have to be taken by the caller
  73      * of this method.

If an I/O error occurs during the operation, then not all characters might have been transferred
and the source or destination could be left in an inconsistent state. The caller of this method
should therefore ensure in such a case that measures are taken to release any resources held
by the source and destination.

On Nov 3, 2017, at 5:49 AM, Alan Bateman <[hidden email]> wrote:

> On 02/11/2017 19:55, Patrick Reinhart wrote:
>> :
>> If we are all happy with the API so far, I could start adding an initial implementation and test…
>>
> I think the API looks fine so go ahead.

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
Hi Brian,

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 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 :-)

-Patrick


> Am 07.11.2017 um 23:38 schrieb Brian Burkhalter <[hidden email]>:
>
> I am a little late to this thread, but some alternative verbiage to consider is included below. This wording however diverges from that of [1] so if we want them to remain similar then it’s probably not worth it to change from what has been agreed upon already.
>
> Thanks,
>
> Brian
>
> [1] https://docs.oracle.com/javase/9/docs/api/java/io/InputStream.html#transferTo-java.io.OutputStream- <https://docs.oracle.com/javase/9/docs/api/java/io/InputStream.html#transferTo-java.io.OutputStream->
>
>   57      * Reads all characters from this source and writes the characters to
>   58      * the given appendable in the order that they are read. On return, the
>   59      * source of characters will be at its end.
>
> Reads all characters from this source and appends them to a destination in
> the order in which they are read. On return, ...
>
>   60      * <p>
>   61      * This method may block indefinitely reading from the readable, or
>   62      * writing to the appendable. Where this source or the appendable is
>   63      * {@link AutoCloseable closeable}, then the behavior when either are
>   64      * <i>asynchronously closed</i>, or the thread interrupted during the transfer,
>   65      * is highly readable and appendable specific, and therefore not specified.
>
> This method may block indefinitely while reading from the source or writing to the destination.
> If the source or destination is {@link AutoCloseable closeable}, then the behavior when either
> is <i>asynchronously closed</i>, or the thread is interrupted during the transfer, is highly
> implementation-dependent and hence unspecified.
>
>   66      * <p>
>   67      * If an I/O error occurs reading from the readable or writing to the
>   68      * appendable, then it may do so after some characters have been read or
>   69      * written. Consequently the readable may not be at end of its data and
>   70      * one, or both participants may be in an inconsistent state. That in mind
>   71      * all additional measures required by one or both participants in order to
>   72      * eventually free their internal resources have to be taken by the caller
>   73      * of this method.
>
> If an I/O error occurs during the operation, then not all characters might have been transferred
> and the source or destination could be left in an inconsistent state. The caller of this method
> should therefore ensure in such a case that measures are taken to release any resources held
> by the source and destination.
>
> On Nov 3, 2017, at 5:49 AM, Alan Bateman <[hidden email] <mailto:[hidden email]>> wrote:
>
>> On 02/11/2017 19:55, Patrick Reinhart wrote:
>>> :
>>> If we are all happy with the API so far, I could start adding an initial implementation and test…
>>>
>> I think the API looks fine so go ahead.
>

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Brian Burkhalter-2
Hi Patrick,

On Nov 7, 2017, at 3:01 PM, Patrick Reinhart <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
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 <[hidden email]>:
>
> Hi Patrick,
>
> On Nov 7, 2017, at 3:01 PM, Patrick Reinhart <[hidden email] <mailto:[hidden email]>> 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

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

roger riggs
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 <[hidden email]>:
>>
>> Hi Patrick,
>>
>> On Nov 7, 2017, at 3:01 PM, Patrick Reinhart <[hidden email] <mailto:[hidden email]>> 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

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
An updated webrev:

http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.01


> 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.
I tried to mention that now.

>
> 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.
I see that point - removed

>
> 96: „in order not having the extra overhead creating" -> "to avoid the extra overhead of"
done

>
> X-Buffer.java.template:
>
> 1555: „form" -> "from"
done

>
> 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().
done

>
> 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"
done

>
>
> On the tests; had you considered using TestNG;
> it provides some good structure and is preferred (AFAIK) for new tests.
To be honest, no. The test for the Readable I basically copied from the InputStream and the one for CharBuffer I just did
not think about… I will certainly consider that for the future :-)

>
> 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())
I will need some digging how to have the RandomFactory be added to the test class path…

>
> Thanks, Roger
>

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

roger riggs
Hi Patrick,

Comments on http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.02:

Readable.java:
74: "to an error" is a bit overloaded, perhaps "to an exception"

X-Buffer.java.template:

1567: "this buffer":  do you mean the "out buffer"

1578:  "The out buffer is this buffer": will read poorly out of context,
perhaps:  "Illegal transfer of a buffer to itself"

1580: Perhaps confine "start" to the else case.

For the tests, an example of using RandomFactory is in
test/jdk/java/io/InputStream/ReadNBytes.java.

Thanks, Roger



On 11/10/2017 4:42 AM, Patrick Reinhart wrote:

> An updated webrev:
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.01
>
>
>> 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.
> I tried to mention that now.
>
>> 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.
> I see that point - removed
>
>> 96: „in order not having the extra overhead creating" -> "to avoid the extra overhead of"
> done
>
>> X-Buffer.java.template:
>>
>> 1555: „form" -> "from"
> done
>
>> 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().
> done
>
>> 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"
> done
>
>>
>> On the tests; had you considered using TestNG;
>> it provides some good structure and is preferred (AFAIK) for new tests.
> To be honest, no. The test for the Readable I basically copied from the InputStream and the one for CharBuffer I just did
> not think about… I will certainly consider that for the future :-)
>
>> 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())
> I will need some digging how to have the RandomFactory be added to the test class path…
>
>> Thanks, Roger
>>

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
Latest changes:

http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.03

-Patrick


> Am 13.11.2017 um 15:35 schrieb Roger Riggs <[hidden email]>:
>
> Hi Patrick,
>
> Comments on http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.02:
>
> Readable.java:
> 74: „to an error" is a bit overloaded, perhaps "to an exception“
done

>
> X-Buffer.java.template:
>
> 1567: „this buffer":  do you mean the "out buffer“
done

>
> 1578:  "The out buffer is this buffer": will read poorly out of context, perhaps:  „Illegal transfer of a buffer to itself“
done

>
> 1580: Perhaps confine "start" to the else case.
>
> For the tests, an example of using RandomFactory is in test/jdk/java/io/InputStream/ReadNBytes.java.
changed the test accordingly


>
> Thanks, Roger
>

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Brian Burkhalter-2
Hi Patrick,

More editorial comments ... ;-)

In Readable.java these lines

  73      * Depending on which class implements the appendable, there may be a limit
  74      * of data that can written to which in turn could lead to an exception.

are a bit unclear to me. Do you mean to say a limit on the amount of data, i.e., number of characters, which may be transferred? If so, then perhaps something like this?:

“Note that it is possible an implementing class may limit the number of characters which may be transferred and throw an exception if this limit is exceeded.”

In X-Buffer.java.template at lines 1556-1561 the following might be considered as an alternative wording:

“The implementation transfers data by one of two means. If the given {@link Appendable} is a {@link CharBuffer}, {@code put(CharBuffer)} is invoked on it with this buffer as parameter. Otherwise {@link Appendable#append(CharSequence, int, int)} is invoked on it with this buffer and its position and length as parameters.”

Thanks,

Brian

On Nov 13, 2017, at 2:23 PM, Patrick Reinhart <[hidden email]> wrote:

> Latest changes:
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.03

12