RFR of 8180451: ByteArrayInputStream should override readAllBytes, readNBytes, and transferTo
https://bugs.openjdk.java.net/browse/JDK-8180451 http://cr.openjdk.java.net/~bpb/8180451/webrev.00 1. Add overrides of readAllBytes(), readNBytes(), and transferTo(). 2. Simplify parameter checks in read(byte[],int,int). 3. Change <code>foo</code> to {@code foo}. Thanks, Brian
On 11 Dec 2017, at 12:47, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
https://bugs.openjdk.java.net/browse/JDK-8180451 http://cr.openjdk.java.net/~bpb/8180451/webrev.00
1. Add overrides of readAllBytes(), readNBytes(), and transferTo(). 2. Simplify parameter checks in read(byte[],int,int). 3. Change <code>foo</code> to {@code foo}.
Minor thing (no need for another review round): 208 public synchronized long transferTo(OutputStream out) throws IOException { 209 int pos0 = pos; 210 out.write(buf, pos, count - pos); 211 pos = count; 212 return count - pos0; 213 } int len = count - pos out.write(but, pos, len); pos = count; return len; Paul.
Thanks,
Brian
On Dec 12, 2017, at 11:42 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:
int len = count - pos out.write(but, pos, len); pos = count; return len;
Indeed that is clearer. Thanks, Brian
Reprising this thread from three months ago [1]. A patch including the changes suggested below is at http://cr.openjdk.java.net/~bpb/8180451/webrev.01/ with the differences between this and the prior version at http://cr.openjdk.java.net/~bpb/8180451/webrev.00-01/ Thanks, Brian [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050550.ht... On Dec 12, 2017, at 11:42 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:
208 public synchronized long transferTo(OutputStream out) throws IOException { 209 int pos0 = pos; 210 out.write(buf, pos, count - pos); 211 pos = count; 212 return count - pos0; 213 }
int len = count - pos out.write(but, pos, len); pos = count; return len;
On Dec 12, 2017, at 12:23 PM, Brent Christian <brent.christian@oracle.com> wrote:
The changes look fine to me. I would have found the test case a little easier to follow if "off"/"len" weren't named so similarly to "offset"/"length", but it's not a big deal.
In: @@ -196,14 +194,32 @@ return len; } + public synchronized byte[] readAllBytes() { + byte[] result = Arrays.copyOfRange(buf, pos, count); + pos = count; + return result; + } + + public synchronized int readNBytes(byte[] b, int off, int len) { + int n = read(b, off, len); + return n == -1 ? 0 : n; + } This probably doesn't need to be synchronized, though I imagine the difference would be minimal. + public synchronized long transferTo(OutputStream out) throws IOException { + int len = count - pos + out.write(but, pos, len); s/but/buf/ I guess? + pos = count; + return len; + } + On Wed, Mar 14, 2018 at 10:31 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Reprising this thread from three months ago [1].
A patch including the changes suggested below is at
http://cr.openjdk.java.net/~bpb/8180451/webrev.01/
with the differences between this and the prior version at
http://cr.openjdk.java.net/~bpb/8180451/webrev.00-01/
Thanks,
Brian
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050550.ht...
On Dec 12, 2017, at 11:42 AM, Paul Sandoz <paul.sandoz@oracle.com> wrote:
208 public synchronized long transferTo(OutputStream out) throws IOException { 209 int pos0 = pos; 210 out.write(buf, pos, count - pos); 211 pos = count; 212 return count - pos0; 213 }
int len = count - pos out.write(but, pos, len); pos = count; return len;
On Dec 12, 2017, at 12:23 PM, Brent Christian <brent.christian@oracle.com> wrote:
The changes look fine to me. I would have found the test case a little easier to follow if "off"/"len" weren't named so similarly to "offset"/"length", but it's not a big deal.
-- - DML
On Mar 14, 2018, at 9:27 AM, David Lloyd <david.lloyd@redhat.com> wrote:
+ public synchronized long transferTo(OutputStream out) throws IOException { + int len = count - pos + out.write(but, pos, len);
s/but/buf/ I guess?
Yes, I already caught that myself. I think I generated the webrev before running tests, i.e., out of sequence. Thanks, Brian
+ pos = count; + return len; + } +
On Mar 14, 2018, at 9:27 AM, David Lloyd <david.lloyd@redhat.com> wrote:
@@ -196,14 +194,32 @@ return len; }
+ public synchronized byte[] readAllBytes() { + byte[] result = Arrays.copyOfRange(buf, pos, count); + pos = count; + return result; + } + + public synchronized int readNBytes(byte[] b, int off, int len) { + int n = read(b, off, len); + return n == -1 ? 0 : n; + }
This probably doesn't need to be synchronized, though I imagine the difference would be minimal.
You are correct, it does not.
+ public synchronized long transferTo(OutputStream out) throws IOException { + int len = count - pos + out.write(but, pos, len);
s/but/buf/ I guess?
Webrevs corrected in place: http://cr.openjdk.java.net/~bpb/8180451/webrev.00-01/ http://cr.openjdk.java.net/~bpb/8180451/webrev.01/ Thanks, Brian
This is still in need of final approval, assuming it is OK. Thanks, Brian On Mar 14, 2018, at 10:50 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Mar 14, 2018, at 9:27 AM, David Lloyd <david.lloyd@redhat.com> wrote:
@@ -196,14 +194,32 @@ return len; }
+ public synchronized byte[] readAllBytes() { + byte[] result = Arrays.copyOfRange(buf, pos, count); + pos = count; + return result; + } + + public synchronized int readNBytes(byte[] b, int off, int len) { + int n = read(b, off, len); + return n == -1 ? 0 : n; + }
This probably doesn't need to be synchronized, though I imagine the difference would be minimal.
You are correct, it does not.
+ public synchronized long transferTo(OutputStream out) throws IOException { + int len = count - pos + out.write(but, pos, len);
s/but/buf/ I guess?
Webrevs corrected in place:
http://cr.openjdk.java.net/~bpb/8180451/webrev.00-01/ http://cr.openjdk.java.net/~bpb/8180451/webrev.01/
Sorry. This looks OK to me (non-reviewer) now. On Wed, Mar 21, 2018 at 1:01 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
This is still in need of final approval, assuming it is OK.
Thanks,
Brian
On Mar 14, 2018, at 10:50 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Mar 14, 2018, at 9:27 AM, David Lloyd <david.lloyd@redhat.com> wrote:
@@ -196,14 +194,32 @@ return len; }
+ public synchronized byte[] readAllBytes() { + byte[] result = Arrays.copyOfRange(buf, pos, count); + pos = count; + return result; + } + + public synchronized int readNBytes(byte[] b, int off, int len) { + int n = read(b, off, len); + return n == -1 ? 0 : n; + }
This probably doesn't need to be synchronized, though I imagine the difference would be minimal.
You are correct, it does not.
+ public synchronized long transferTo(OutputStream out) throws IOException { + int len = count - pos + out.write(but, pos, len);
s/but/buf/ I guess?
Webrevs corrected in place:
http://cr.openjdk.java.net/~bpb/8180451/webrev.00-01/ http://cr.openjdk.java.net/~bpb/8180451/webrev.01/
-- - DML
Hi Brian, Looks fine. Cosmetically, you could join some of the lines in ByteArrayInputStream 153-164. In the test, ReadAllReadNTransferTo.java 53-54: spaces around "/" and before "-" No further review needed. Thanks, Roger On 3/21/2018 2:01 PM, Brian Burkhalter wrote:
This is still in need of final approval, assuming it is OK.
Thanks,
Brian
On Mar 14, 2018, at 10:50 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Mar 14, 2018, at 9:27 AM, David Lloyd <david.lloyd@redhat.com> wrote:
@@ -196,14 +194,32 @@ return len; }
+ public synchronized byte[] readAllBytes() { + byte[] result = Arrays.copyOfRange(buf, pos, count); + pos = count; + return result; + } + + public synchronized int readNBytes(byte[] b, int off, int len) { + int n = read(b, off, len); + return n == -1 ? 0 : n; + }
This probably doesn't need to be synchronized, though I imagine the difference would be minimal. You are correct, it does not.
+ public synchronized long transferTo(OutputStream out) throws IOException { + int len = count - pos + out.write(but, pos, len);
s/but/buf/ I guess? Webrevs corrected in place:
http://cr.openjdk.java.net/~bpb/8180451/webrev.00-01/ http://cr.openjdk.java.net/~bpb/8180451/webrev.01/
Hi Roger, Thanks for the review. I’ll update it prior to checking it in. Brian On Mar 23, 2018, at 8:35 AM, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Looks fine.
Cosmetically, you could join some of the lines in ByteArrayInputStream 153-164.
In the test, ReadAllReadNTransferTo.java 53-54: spaces around "/" and before "-"
No further review needed.
Hi, Brian The changes look fine to me. I would have found the test case a little easier to follow if "off"/"len" weren't named so similarly to "offset"/"length", but it's not a big deal. -Brent On 12/11/17 12:47 PM, Brian Burkhalter wrote:
https://bugs.openjdk.java.net/browse/JDK-8180451 http://cr.openjdk.java.net/~bpb/8180451/webrev.00
1. Add overrides of readAllBytes(), readNBytes(), and transferTo(). 2. Simplify parameter checks in read(byte[],int,int). 3. Change <code>foo</code> to {@code foo}.
Thanks,
Brian
participants (5)
-
Brent Christian
-
Brian Burkhalter
-
David Lloyd
-
Paul Sandoz
-
Roger Riggs