Updates SourceBufferingInputStream to conform to InputStream contract #1891
Conversation
|
This specifically looks at |
|
It should be noted that this comes from #1890 and makes |
|
|
||
| SourceBufferingInputStream(BufferedSource source) { | ||
| this.source = source; | ||
| this.buffer = source.buffer(); | ||
| } | ||
|
|
||
| private final Buffer temp = new Buffer(); | ||
| // offset is the write offset in the dest array |
There was a problem hiding this comment.
Can you move this comment inside the method so it doesn't get lost if shuffling occurs. Also please capitalize "offset" and add a period. Comments should be sentences.
| private int copyTo(byte[] sink, int offset, int byteCount) { | ||
| // TODO replace this with https://github.com/square/okio/issues/362 | ||
| buffer.copyTo(temp, offset, byteCount); | ||
| buffer.copyTo(temp, position, byteCount); |
There was a problem hiding this comment.
I meant to circle back and fix this in Picasso, but I noticed this was a bug when trying to add copyTo to Okio. It becomes really annoying because we need two offsets in the API.
| mark = -1; | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| @Override public int read(@NonNull byte[] b, int off, int len) throws IOException { | ||
| source.require(position + len); | ||
| int copied = /*buffer.*/copyTo(b, off, len); | ||
| checkNotNull(b, "b is null"); |
There was a problem hiding this comment.
Although I'm not sure we really need this explicit check. b is immediately dereferenced by the if statement below.
| @@ -33,33 +35,49 @@ | |||
| private final Buffer buffer; | |||
| private long position; | |||
| private long mark = -1; | |||
There was a problem hiding this comment.
Should we call this markPosition then?
|
|
||
| len = stream.read(bytes); | ||
| assertEquals(3, len); | ||
| // last two bytes are out of range so untouched from previous run |
There was a problem hiding this comment.
nit: capitalize, period
| stream.reset(); | ||
| fail("expected IOException on reset"); | ||
| } catch (IOException e) { | ||
| // success |
There was a problem hiding this comment.
we tend to name the catch variable "expected" so that we can omit this comment
There was a problem hiding this comment.
Or you could assert on the message to make sure we're getting the correct IOE
assertEquals("No mark or whatever", e.getMessage());There was a problem hiding this comment.
The InputStream documentation says might a lot when referring to throwing exceptions here, so I figured that the error message might change to something more fine grained for separate cases while this test is still valid, so wanted to avoid checking the message content.
| @@ -42,34 +43,48 @@ | |||
| private final Buffer temp = new Buffer(); | |||
| private int copyTo(byte[] sink, int offset, int byteCount) { | |||
| // TODO replace this with https://github.com/square/okio/issues/362 | |||
| buffer.copyTo(temp, offset, byteCount); | |||
| // `copyTo` treats offset as the read position, `read` treats offset as the write offset. | |||
|
Thanks! |
No description provided.