6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Andrew Luo]

I'm not a reviewer/committer but just a suggestion:

You can use assert to ensure "document" that skip(long) should never return negative (and also provide useful debugging when assertions are enabled), for example:

586             long ns = skip(n);
assert ns >= 0 : "skip(long) returned negative value " + ns;
 587 
 588             // If not enough skipped, read and discard bytes, failing on EOF
 589             if (ns != n) {
 590                 discardNBytes(n - ns, true);
 591             }

Thanks,
Andrew

-----Original Message-----
From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> On Behalf Of Brent Christian
Sent: Wednesday, October 24, 2018 12:08 PM
To: Brian Burkhalter <brian.burkhalter at oracle.com>; Java Core Libs <core-libs-dev at openjdk.java.net>
Subject: Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

Hi,

I think that's looking pretty good.  One thing:

InputStream.java

  515      * @param      n   the number of bytes to be skipped.

How about an @param for throwOnEOF, too?


Regarding the tests:

NullInputStream.java

  237     @Test(groups = "closed")
  238     public static void testSkipNBytesClosed() {

Can this method also use the "expectedExceptions" attribute of @Test ?

--
Skip.java

Is there a test for skipNBytes() where the skip() call skips < n bytes ? 
  I'm thinking about code coverage of:

  588             // If not enough skipped, read and discard bytes, 
failing on EOF
  589             if (ns != n) {
  590                 discardNBytes(n - ns, true);
  591             }

If not, maybe MyInputStream could override skip(long), with a mode where it would skip < n bytes.

Thanks,
-Brent

On 10/23/18 3:02 PM, Brian Burkhalter wrote:
> Here is an updated version which hopefully addresses the shortcoming 
> pointed out below:
> 
> http://cr.openjdk.java.net/~bpb/6516099/webrev.02/
> <http://cr.openjdk.java.net/%7Ebpb/6516099/webrev.02/>
> 
> The remaining assumption here is that skip(long) never returns a 
> negative value.
> 
> Thanks,
> 
> Brian
> 
>> On Oct 23, 2018, at 11:29 AM, Brian Burkhalter 
>> <brian.burkhalter at oracle.com <mailto:brian.burkhalter at oracle.com>> wrote:
>>
>> That is exactly why version .00 of the patch implemented skipNBytes() 
>> in a fixed way in terms of read(). I think that there is a good 
>> compromise however and I shall update the patch accordingly.
>>
>>> On Oct 22, 2018, at 12:55 PM, Brent Christian 
>>> <brent.christian at oracle.com <mailto:brent.christian at oracle.com>> wrote:
>>>
>>> 562     public void skipNBytes(long n) throws IOException {
>>> 563         if (n > 0 && skip(n) != n) {
>>> 564             throw new EOFException("End of stream before enough 
>>> bytes skipped");
>>> 565         }
>>> 566     }
>>>
>>> If an overrided skip() method were to skip < n bytes but not yet be 
>>> at EOF, which I think the skip() spec allows, then wouldn't
>>> skipNBytes() throw an EOFException ?
>