JDK-8067661: transferTo proposal for Appendable

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

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart
Hi Brian

See latest changes here:

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

-Patrick

> Am 14.11.2017 um 01:34 schrieb Brian Burkhalter <[hidden email]>:
>
> Hi Patrick,
>
> More editorial comments ... ;-)
>

No problem :-)

> 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.”

That describes it better than the actual version and therefore changed

>
> 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.”
>

Seems also improve the wording - changed

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

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Alan Bateman
On 14/11/2017 07:51, Patrick Reinhart wrote:
> Hi Brian
>
> See latest changes here:
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.03
>
This version mostly looks quite good, a few comments:

- Readable "hence unspecified". In several other places, including
InputStream.transferTo, we use "therefore unspecified" and it would be
good to keep that consistent.

- Readable "Depending on which class implements the appendable ...".
What would you think about replacing this sentence with: "If the
destination is capacity bounded and has insufficient capacity to append
all characters read from the source then an exception will be thrown
after transferring zero or some bytes to the destination".

- CharBuffer. I don't think the first sentence in the @implSpec is
needed, it's okay to start with "If the given Appendable ...". There's a
typo in the second sentence "it's data if possible" -> "its data" (no
"if possible" as it will use put unconditionally when the destination is
a CharBuffer).

- CharBuffer. IOException is not possible when reading here, it's only
writing.

- CharBuffer: NullPointerException is covered by the package
description, the methods in this package do not specified NPE. It is not
needed in the read method either.

- CharBuffer: The @param and @throws have a specific style in this
package and probably best to keep that consistent if you can.

-Alan



Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Brian Burkhalter-2

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

> - Readable "hence unspecified". In several other places, including InputStream.transferTo, we use "therefore unspecified" and it would be good to keep that consistent.

Mea culpa on the “hence.”

> - Readable "Depending on which class implements the appendable ...". What would you think about replacing this sentence with: "If the destination is capacity bounded and has insufficient capacity to append all characters read from the source then an exception will be thrown after transferring zero or some bytes to the destination".

s/zero or some/zero or more/

Brian
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart


> Am 14.11.2017 um 16:33 schrieb Brian Burkhalter <[hidden email]>:
>
>
> On Nov 14, 2017, at 3:45 AM, Alan Bateman <[hidden email] <mailto:[hidden email]>> wrote:
>
>> - Readable "hence unspecified". In several other places, including InputStream.transferTo, we use "therefore unspecified" and it would be good to keep that consistent.
>
> Mea culpa on the “hence.”

So I will change „hence unspecified“ back to „therefore unspecified“ here..

>
>> - Readable "Depending on which class implements the appendable ...". What would you think about replacing this sentence with: "If the destination is capacity bounded and has insufficient capacity to append all characters read from the source then an exception will be thrown after transferring zero or some bytes to the destination".
>
> s/zero or some/zero or more/
>

And what should it be then here? I’m a bit confused now…

-Patrick

Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Alan Bateman


On 14/11/2017 16:59, Patrick Reinhart wrote:
> :
>
> And what should it be then here? I’m a bit confused now…
>
Sorry, there was a typo in my mail that Brian picked up on. I think we
are converging on:

"If the destination is capacity bounded and has insufficient capacity to
append all characters read from the source then the exception may be
thrown after transferring some characters to the destination".

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

Re: JDK-8067661: transferTo proposal for Appendable

Brian Burkhalter-2
On Nov 15, 2017, at 3:22 AM, Alan Bateman <[hidden email]> wrote:

> "If the destination is capacity bounded and has insufficient capacity to append all characters read from the source then the exception may be thrown after transferring some characters to the destination".

Could “is capacity-bounded and” be removed?

Brian
Reply | Threaded
Open this post in threaded view
|

Re: JDK-8067661: transferTo proposal for Appendable

Patrick Reinhart

Just to see if I picked up the Discussion so far. The discussion is
about that original sentence
from: http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.03

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

And your current suggestion would look like:

1) "If the destination is capacity bounded and has insufficient capacity
to append all characters
read from the source then the exception may be thrown after transferring
some characters
to the destination"

At the version from:
http://cr.openjdk.java.net/~reinhapa/reviews/8067661/webrev.04
I got the version from Brian Burkhalter suggested earlier:

2) "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."


Now looking at both versions, the version 1 seems a bit more declarative
than version 2 and I
would go for the first one...

-Patrick



Am 15.11.2017 um 19:07 schrieb Brian Burkhalter:
> On Nov 15, 2017, at 3:22 AM, Alan Bateman <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>> .
>
> Could “is capacity-bounded and” be removed?
>
> Brian


12