RFR: 8247918: Clarify Reader.skip behavior for end of stream

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

RFR: 8247918: Clarify Reader.skip behavior for end of stream

Brian Burkhalter-3
Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.

-------------

Commit messages:
 - 8247918: Clarify Reader.skip behavior for end of stream

Changes: https://git.openjdk.java.net/jdk/pull/2274/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2274&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8247918
  Stats: 132 lines in 8 files changed: 62 ins; 38 del; 32 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2274.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2274/head:pull/2274

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream

Brian Burkhalter-3
On Thu, 28 Jan 2021 02:22:55 GMT, Brian Burkhalter <[hidden email]> wrote:

> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.

ping

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream

Naoto Sato-2
In reply to this post by Brian Burkhalter-3
On Thu, 28 Jan 2021 02:22:55 GMT, Brian Burkhalter <[hidden email]> wrote:

> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.

src/java.base/share/classes/java/io/FilterReader.java line 81:

> 79:      * {@inheritDoc}
> 80:      *
> 81:      * @throws     IllegalArgumentException  If {@code n} is negative and the

Does this have to be different from the `Reader.skip()`'s description? Since the contained reader implements `Reader` (throws IAE as a contract), that condition after `and` is always true?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream

Brian Burkhalter-3
On Mon, 8 Feb 2021 22:51:43 GMT, Naoto Sato <[hidden email]> wrote:

>> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.
>
> src/java.base/share/classes/java/io/FilterReader.java line 81:
>
>> 79:      * {@inheritDoc}
>> 80:      *
>> 81:      * @throws     IllegalArgumentException  If {@code n} is negative and the
>
> Does this have to be different from the `Reader.skip()`'s description? Since the contained reader implements `Reader` (throws IAE as a contract), that condition after `and` is always true?

This came from some `Reader`s, e.g., `CharArrayReader`, `StringReader`, overriding `skip()` _not_ to throw an IAE. But at the specification level perhaps this should not be recognized.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream

Naoto Sato-2
On Mon, 8 Feb 2021 22:58:13 GMT, Brian Burkhalter <[hidden email]> wrote:

>> src/java.base/share/classes/java/io/FilterReader.java line 81:
>>
>>> 79:      * {@inheritDoc}
>>> 80:      *
>>> 81:      * @throws     IllegalArgumentException  If {@code n} is negative and the
>>
>> Does this have to be different from the `Reader.skip()`'s description? Since the contained reader implements `Reader` (throws IAE as a contract), that condition after `and` is always true?
>
> This came from some `Reader`s, e.g., `CharArrayReader`, `StringReader`, overriding `skip()` _not_ to throw an IAE. But at the specification level perhaps this should not be recognized.

Makes sense. Then I would let CSR decide whether to include the description or not.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v2]

Brian Burkhalter-3
In reply to this post by Brian Burkhalter-3
> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.

Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:

  8247918: Change'ns' to 'n' in the skip doc

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2274/files
  - new: https://git.openjdk.java.net/jdk/pull/2274/files/0ebed6ea..2a2ceb41

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2274&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2274&range=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2274.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2274/head:pull/2274

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v2]

Roger Riggs-2
On Fri, 12 Feb 2021 01:18:53 GMT, Brian Burkhalter <[hidden email]> wrote:

>> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.
>
> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>
>   8247918: Change'ns' to 'n' in the skip doc

The use of @ inheritdoc causes some unexpected changes in the javadoc, dropping the skip method from several classes.

src/java.base/share/classes/java/io/StringReader.java line 108:

> 106:
> 107:     /**
> 108:      * {@inheritDoc}

Using inheritDoc for the exceptions adds several new and irrelevant impossible causes to the javadoc.
" This method will block until some characters are available", etc.

src/java.base/share/classes/java/io/PushbackReader.java line 257:

> 255:
> 256:     /**
> 257:      * {@inheritDoc}

This appears to make the skip method disappear from the PushbackReader javadoc
and only show up as a method declared in FilterReader.

src/java.base/share/classes/java/io/LineNumberReader.java line 270:

> 268:
> 269:     /**
> 270:      * {@inheritDoc}

This appears to make the skip method disappear from the LineNumberReader javadoc
and only show up as a method declared in Reader.

src/java.base/share/classes/java/io/CharArrayReader.java line 149:

> 147:
> 148:     /**
> 149:      * {@inheritDoc}

Using inheritDoc for the exceptions adds several new and irrelevant impossible causes to the javadoc.
" This method will block until some characters are available", etc.
It also drops the existing "If the stream is closed" condition on @ throws IOException.

src/java.base/share/classes/java/io/BufferedReader.java line 400:

> 398:
> 399:     /**
> 400:      * {@inheritDoc}

This appears to make the skip method disappear from the BufferedReader javadoc
and only show up as a method declared in Reader.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v2]

Brian Burkhalter-3
On Fri, 12 Feb 2021 15:51:59 GMT, Roger Riggs <[hidden email]> wrote:

>> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8247918: Change'ns' to 'n' in the skip doc
>
> src/java.base/share/classes/java/io/StringReader.java line 108:
>
>> 106:
>> 107:     /**
>> 108:      * {@inheritDoc}
>
> Using inheritDoc for the exceptions adds several new and irrelevant impossible causes to the javadoc.
> " This method will block until some characters are available", etc.

Good catch; will fix.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v2]

Brian Burkhalter-3
In reply to this post by Roger Riggs-2
On Fri, 12 Feb 2021 16:03:09 GMT, Roger Riggs <[hidden email]> wrote:

>> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8247918: Change'ns' to 'n' in the skip doc
>
> src/java.base/share/classes/java/io/LineNumberReader.java line 270:
>
>> 268:
>> 269:     /**
>> 270:      * {@inheritDoc}
>
> This appears to make the skip method disappear from the LineNumberReader javadoc
> and only show up as a method declared in Reader.

Right. This is intended and expected.

> src/java.base/share/classes/java/io/CharArrayReader.java line 149:
>
>> 147:
>> 148:     /**
>> 149:      * {@inheritDoc}
>
> Using inheritDoc for the exceptions adds several new and irrelevant impossible causes to the javadoc.
> " This method will block until some characters are available", etc.
> It also drops the existing "If the stream is closed" condition on @ throws IOException.

As above for `StringReader`.

> src/java.base/share/classes/java/io/PushbackReader.java line 257:
>
>> 255:
>> 256:     /**
>> 257:      * {@inheritDoc}
>
> This appears to make the skip method disappear from the PushbackReader javadoc
> and only show up as a method declared in FilterReader.

This is intended and expected.

> src/java.base/share/classes/java/io/BufferedReader.java line 400:
>
>> 398:
>> 399:     /**
>> 400:      * {@inheritDoc}
>
> This appears to make the skip method disappear from the BufferedReader javadoc
> and only show up as a method declared in Reader.

This is intended and expected.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v3]

Brian Burkhalter-3
In reply to this post by Brian Burkhalter-3
> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.

Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:

  8247918: Remove verbiage describing impossible behavior in CharArrayReader and StringReader

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2274/files
  - new: https://git.openjdk.java.net/jdk/pull/2274/files/2a2ceb41..a33245d3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2274&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2274&range=01-02

  Stats: 17 lines in 2 files changed: 15 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2274.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2274/head:pull/2274

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v2]

liach
In reply to this post by Roger Riggs-2
On Fri, 12 Feb 2021 16:11:58 GMT, Roger Riggs <[hidden email]> wrote:

>> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8247918: Change'ns' to 'n' in the skip doc
>
> The use of @ inheritdoc causes some unexpected changes in the javadoc, dropping the skip method from several classes.

I personally suggest moving this clarification into a separate paragraph as opposed to appending it to the first sentence.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v2]

Brian Burkhalter-2
I would be all right with that.

> On Feb 12, 2021, at 9:45 AM, liach <[hidden email]> wrote:
>
> On Fri, 12 Feb 2021 16:11:58 GMT, Roger Riggs <[hidden email]> wrote:
>
>>> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>  8247918: Change'ns' to 'n' in the skip doc
>>
>> The use of @ inheritdoc causes some unexpected changes in the javadoc, dropping the skip method from several classes.
>
> I personally suggest moving this clarification into a separate paragraph as opposed to appending it to the first sentence.
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2274

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v3]

Roger Riggs-2
In reply to this post by Brian Burkhalter-3
On Fri, 12 Feb 2021 17:25:00 GMT, Brian Burkhalter <[hidden email]> wrote:

>> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.
>
> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>
>   8247918: Remove verbiage describing impossible behavior in CharArrayReader and StringReader

Marked as reviewed by rriggs (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8247918: Clarify Reader.skip behavior for end of stream [v3]

Naoto Sato-2
In reply to this post by Brian Burkhalter-3
On Fri, 12 Feb 2021 17:25:00 GMT, Brian Burkhalter <[hidden email]> wrote:

>> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.
>
> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>
>   8247918: Remove verbiage describing impossible behavior in CharArrayReader and StringReader

Marked as reviewed by naoto (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274
Reply | Threaded
Open this post in threaded view
|

Integrated: 8247918: Clarify Reader.skip behavior for end of stream

Brian Burkhalter-3
In reply to this post by Brian Burkhalter-3
On Thu, 28 Jan 2021 02:22:55 GMT, Brian Burkhalter <[hidden email]> wrote:

> Please review this clarification of the specification of the method `skip(long)` in `java.io.Reader` and its subclasses. Specifically, the behavior of the method is made clear for the case when the `Reader` is already at the end of its stream when the method is invoked. A corresponding CSR will be filed. Also, the change includes an update to an existing test in order to verify that the specification change reflects actual behavior.

This pull request has now been integrated.

Changeset: 7ffa1481
Author:    Brian Burkhalter <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/7ffa1481
Stats:     138 lines in 8 files changed: 69 ins; 30 del; 39 mod

8247918: Clarify Reader.skip behavior for end of stream

Reviewed-by: rriggs, naoto

-------------

PR: https://git.openjdk.java.net/jdk/pull/2274