Re: RFR 4358774: Add null InputStream and OutputStream

On Jan 12, 2018, at 10:12 AM, Alan Bateman wrote: > On 11/01/2018 22:14, Brian Burkhalter wrote: >> Thanks. The final version is updated in place under webrev.03. Pushing the >> change is pending CSR-reapproval. >> >> > Looks okay to me. Just a minor nit in the first

Re: RFR 4358774: Add null InputStream and OutputStream

On 11/01/2018 22:14, Brian Burkhalter wrote: Thanks. The final version is updated in place under webrev.03. Pushing the change is pending CSR-reapproval. Looks okay to me. Just a minor nit in the first line of nullInputStream where the javadoc has "contains no bytes". I assume you mean to

Re: RFR 4358774: Add null InputStream and OutputStream

Thanks. The final version is updated in place under webrev.03. Pushing the change is pending CSR-reapproval. Brian On Jan 11, 2018, at 11:16 AM, Roger Riggs wrote: > +1 looks fine > > On 1/11/2018 11:59 AM, Brian Burkhalter wrote: >> While there appears now to be

Re: RFR 4358774: Add null InputStream and OutputStream

+1 looks fine On 1/11/2018 11:59 AM, Brian Burkhalter wrote: While there appears now to be agreement on using naming option C below, formal Reviewer approval appears still to be lacking. It would be good if there were at least two approvals forthcoming, if appropriate.

Re: RFR 4358774: Add null InputStream and OutputStream

While there appears now to be agreement on using naming option C below, formal Reviewer approval appears still to be lacking. It would be good if there were at least two approvals forthcoming, if appropriate. http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ Thanks, Brian On Jan 4, 2018, at

Re: RFR 4358774: Add null InputStream and OutputStream

That’s my favorite too… -Patrick > Am 05.01.2018 um 16:31 schrieb Roger Riggs : > > +1 > > C) InputStream.nullInputStream() and OutputStream.nullOutputStream().

Re: RFR 4358774: Add null InputStream and OutputStream

+1 C) InputStream.nullInputStream() and OutputStream.nullOutputStream(). On 1/5/2018 3:42 AM, Chris Hegarty wrote: On 4 Jan 2018, at 23:42, Brian Burkhalter wrote: On Dec 11, 2017, at 12:52 PM, Brian Burkhalter wrote: On Dec 8,

Re: RFR 4358774: Add null InputStream and OutputStream

> On 4 Jan 2018, at 23:42, Brian Burkhalter wrote: > >> On Dec 11, 2017, at 12:52 PM, Brian Burkhalter >> wrote: >> >>> On Dec 8, 2017, at 3:12 PM, Brian Burkhalter >>> wrote: >>> >>> All previous

Re: RFR 4358774: Add null InputStream and OutputStream

On Dec 11, 2017, at 12:52 PM, Brian Burkhalter wrote: > On Dec 8, 2017, at 3:12 PM, Brian Burkhalter > wrote: > >> All previous suggested changes have been made in >> >> http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ >> >>

Re: RFR 4358774: Add null InputStream and OutputStream

On Dec 8, 2017, at 3:12 PM, Brian Burkhalter wrote: > All previous suggested changes have been made in > > http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ > > except for the possible change of name for the IS and OS nullStream() methods > which awaits

Re: RFR 4358774: Add null InputStream and OutputStream

Sorry, missed that thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050458.html -Patrick Am 09.12.2017 um 22:28 schrieb Patrick Reinhart: > Hi Brian, > >> All previous suggested changes have been made in >> >> http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ >> >

Re: RFR 4358774: Add null InputStream and OutputStream

Hi Brian, > All previous suggested changes have been made in > > http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ > 99 @Override 100 public int read(byte[] b, int off, int len) throws IOException { 101 Objects.checkFromIndexSize(off, len,

Re: RFR 4358774: Add null InputStream and OutputStream

On Dec 8, 2017, at 5:06 PM, Sergey Bylokhov wrote: > On 08/12/2017 16:49, Brian Burkhalter wrote: >> I agree it looks strange but it is intentional as it matches the existing >> InputStream.read(byte[],int,in) [1]. (I will remove line 167 as part of this >> patch.)

Re: RFR 4358774: Add null InputStream and OutputStream

Hi Brent, On Dec 8, 2017, at 4:50 PM, Brent Christian wrote: > I just noticed a couple small things in the test: > > test/jdk/java/io/InputStream/NullInputStream.java > > […] > > Line 113 should read "... != 0", yes? > > […] > > On 132, it's "skip() != 0". You

Re: RFR 4358774: Add null InputStream and OutputStream

On 08/12/2017 16:49, Brian Burkhalter wrote: I agree it looks strange but it is intentional as it matches the existing InputStream.read(byte[],int,in) [1]. (I will remove line 167 as part of this patch.) Note that the IOE for the stream being closed would not be thrown in the current code

Re: RFR 4358774: Add null InputStream and OutputStream

Hi, Brian On 12/8/17 3:12 PM, Brian Burkhalter wrote: http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ The changes look good to me. I just noticed a couple small things in the test: test/jdk/java/io/InputStream/NullInputStream.java 109 @Test(groups = "open") 110 public

Re: RFR 4358774: Add null InputStream and OutputStream

Hi Sergey, On Dec 8, 2017, at 4:34 PM, Sergey Bylokhov wrote: > One more issue that according to the spec the new method > read(byte[], int, int) should throw an exception if the stream was closed, > but as far as I understand it can return "0" if "len=0" even if

Re: RFR 4358774: Add null InputStream and OutputStream

Hi, Brian. On 08/12/2017 15:12, Brian Burkhalter wrote: All previous suggested changes have been made in http://cr.openjdk.java.net/~bpb/4358774/webrev.03/ One more issue that according to the spec the new method read(byte[], int, int) should throw an exception if the stream was closed, but

Re: RFR 4358774: Add null InputStream and OutputStream

On Dec 8, 2017, at 8:29 AM, Brian Burkhalter wrote: > On Dec 8, 2017, at 4:39 AM, Alan Bateman wrote: > >>> http://cr.openjdk.java.net/~bpb/4358774/webrev.02/. >>> >> I read through the javadoc again and I think it looks good. >> >> On

Re: RFR 4358774: Add null InputStream and OutputStream

I like the nullInputStream() nullOutputStream() as I would first search for those names, also nullReader() / nullWriter() seem to fit more than Sink/Source or Stream -Patrick > Am 08.12.2017 um 19:38 schrieb Brian Burkhalter : > > Patrick’s comment made us think

Re: RFR 4358774: Add null InputStream and OutputStream

On Fri, Dec 8, 2017 at 1:03 PM, Brian Burkhalter wrote: > On Dec 8, 2017, at 10:52 AM, David Lloyd wrote: > >> On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter >> wrote: >>> Patrick’s comment made us think again

Re: RFR 4358774: Add null InputStream and OutputStream

Like David, I prefer the `null{$ClassName}` alternative to `null{Source|Sink}`. (I assume from his wording that he prefers `null{$ClassName}`.) I prefer `null{$ClassName}` not only because it's less ambiguous, but also because Guava has existing types `{Byte|Char}Source` and `{Byte|Char}Sink`, so

Re: RFR 4358774: Add null InputStream and OutputStream

On Dec 8, 2017, at 10:52 AM, David Lloyd wrote: > On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter > wrote: >> Patrick’s comment made us think again about the naming here as >> “nullStream()” would not fit for eventual equivalent methods on

Re: RFR 4358774: Add null InputStream and OutputStream

On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter wrote: > Patrick’s comment made us think again about the naming here as “nullStream()” > would not fit for eventual equivalent methods on Reader and Writer. It might > be better to go with something like > >

Re: RFR 4358774: Add null InputStream and OutputStream

Patrick’s comment made us think again about the naming here as “nullStream()” would not fit for eventual equivalent methods on Reader and Writer. It might be better to go with something like InputStream InputStream.nullSource(); OutputStream.nullSink(); and later Reader.nullSource();

Re: RFR 4358774: Add null InputStream and OutputStream

On Dec 8, 2017, at 4:39 AM, Alan Bateman wrote: >> http://cr.openjdk.java.net/~bpb/4358774/webrev.02/. >> > I read through the javadoc again and I think it looks good. > > On the implementation then Sergey has a point, the requireNonNull isn't > needed to check b when

Re: RFR 4358774: Add null InputStream and OutputStream

On 07/12/2017 22:55, Brian Burkhalter wrote: Hi Roger, I agree. It does not seem that whatever performance improvement might accrue from not using the Objects methods is offset by the increased code readability in addition to mitigating the risks mentioned in [1]. I have reinstated this

Re: RFR 4358774: Add null InputStream and OutputStream

Hi Sergey, On Dec 7, 2017, at 3:14 PM, Sergey Bylokhov wrote: >> http://cr.openjdk.java.net/~bpb/4358774/webrev.02/. > > Why the >Objects.requireNonNull(b); > should be called before >Objects.checkFromIndexSize(off, len, b.length); > since the second call

Re: RFR 4358774: Add null InputStream and OutputStream

On 07/12/2017 14:55, Brian Burkhalter wrote: Hi Roger, I agree. It does not seem that whatever performance improvement might accrue from not using the Objects methods is offset by the increased code readability in addition to mitigating the risks mentioned in [1]. I have reinstated this

Re: RFR 4358774: Add null InputStream and OutputStream

Hi Roger, I agree. It does not seem that whatever performance improvement might accrue from not using the Objects methods is offset by the increased code readability in addition to mitigating the risks mentioned in [1]. I have reinstated this approach in

Re: RFR 4358774: Add null InputStream and OutputStream

Hi Brian, For checking indices, I think you should leverage the work done for java.util.Objects.checkFromIndexSize(...) as optimized for this purpose in 8135248.  That was extensively designed and reviewed for optimal performance. Regards, Roger [1]

Re: RFR 4358774: Add null InputStream and OutputStream

Updated patch: http://cr.openjdk.java.net/~bpb/4358774/webrev.01/ On Dec 7, 2017, at 6:08 AM, Alan Bateman wrote: > If nothing else, a private ensureOpen method would make it easier to read so > that the "if (closed) throw ..." isn't needed in every method. Done. On

Re: RFR 4358774: Add null InputStream and OutputStream

> On Dec 6, 2017, at 11:11 AM, Jonathan Gibbons > wrote: > >> "null" is a significant term in the Java ecosystem, and the relationship >> here, to /dev/null or NUL seems somewhat tenuous. >> >> Have any other names been considered? At least for the InputStream,

Re: RFR 4358774: Add null InputStream and OutputStream

rmance angle, I'd be more concerned with the calls to > > Objects.xyz() methods there. Unless something has changed in the JIT > > recently, those are susceptible to profile pollution and can cause missed > > optimizations. I'd inline those methods manually to give these methods > >

Re: RFR 4358774: Add null InputStream and OutputStream

heir own profiles. Jason From: Brian Burkhalter <brian.burkhal...@oracle.com> Sent: Wednesday, December 6, 2017 2:05 PM To: Jason Mehrens Cc: core-libs-dev Subject: Re: RFR 4358774: Add null InputStream and OutputStream Jason, On Dec 6, 2017, at 11:54 AM, Jason Mehrens <jason_meh

Re: RFR 4358774: Add null InputStream and OutputStream

On 06/12/2017 21:01, Jason Mehrens wrote: Brian, My understand is JDK-6533165 is moving the bytes that are rarely invoked to a cold method. Therefore: if (closed) { throw new IOException("Stream closed"); } ==becomes=== if (closed) { throw sc(); } private static IOException

Re: RFR 4358774: Add null InputStream and OutputStream

__ > From: Brian Burkhalter <brian.burkhal...@oracle.com> > Sent: Wednesday, December 6, 2017 2:05 PM > To: Jason Mehrens > Cc: core-libs-dev > Subject: Re: RFR 4358774: Add null InputStream and OutputStream > > Jason, > > On Dec 6, 2017, at 11:54 AM, Jason Mehrens <jas

Re: RFR 4358774: Add null InputStream and OutputStream

voked multiple times. Jason From: Brian Burkhalter <brian.burkhal...@oracle.com> Sent: Wednesday, December 6, 2017 2:05 PM To: Jason Mehrens Cc: core-libs-dev Subject: Re: RFR 4358774: Add null InputStream and OutputStream Jason, On Dec 6, 2017,

Re: RFR 4358774: Add null InputStream and OutputStream

On Dec 6, 2017, at 12:46 PM, Patrick Reinhart wrote: >> Yes I still need to look through the JDK source base for some code to >> replace with this, assuming this is approved. > > You got min - even I’m not eligible to give you an official one :-) Thanks. >> Is there also a

Re: RFR 4358774: Add null InputStream and OutputStream

> Am 06.12.2017 um 21:43 schrieb Brian Burkhalter : > > On Dec 6, 2017, at 12:36 PM, Patrick Reinhart > wrote: > >> Sounds great, perfect to remove some more own code… > > Yes I still need to look through the JDK

Re: RFR 4358774: Add null InputStream and OutputStream

On Dec 6, 2017, at 12:36 PM, Patrick Reinhart wrote: > Sounds great, perfect to remove some more own code… Yes I still need to look through the JDK source base for some code to replace with this, assuming this is approved. > Is there also a issue for the same kind of

Re: RFR 4358774: Add null InputStream and OutputStream

Sounds great, perfect to remove some more own code… Is there also a issue for the same kind of methods for Reader and Writer? -Patrick > Am 06.12.2017 um 20:00 schrieb Brian Burkhalter : > > https://bugs.openjdk.java.net/browse/JDK-4358774 >

Re: RFR 4358774: Add null InputStream and OutputStream

Jason, On Dec 6, 2017, at 11:54 AM, Jason Mehrens wrote: > For nullInputStream would it make any sense to use the ByteArrayInputStream > with a (private static) empty byte array? Maybe 'return new > ByteArrayInputStream("".getBytes());'? One side effect is that

Re: RFR 4358774: Add null InputStream and OutputStream

cember 6, 2017 1:00 PM To: core-libs-dev Subject: RFR 4358774: Add null InputStream and OutputStream https://bugs.openjdk.java.net/browse/JDK-4358774 http://cr.openjdk.java.net/~bpb/4358774/webrev.00/ Add nullStream() method to each of InputStream and OutputStream. Thanks, Brian

Re: RFR 4358774: Add null InputStream and OutputStream

The name “emptyStream()” was considered for InputStream and “discardingStream()” for OutputStream. It was thought that “null” or “empty” would be more likely to be found by developers due to familiarity. FWIW there is precedent in third party libraries for the “null” names. Brian On Dec 6,

Re: RFR 4358774: Add null InputStream and OutputStream

"null" is a significant term in the Java ecosystem, and the relationship here, to /dev/null or NUL seems somewhat tenuous. Have any other names been considered?  At least for the InputStream, calling it an "empty stream" seems more intuitive than a "null stream". -- Jon On 12/6/17 11:00

RFR 4358774: Add null InputStream and OutputStream

https://bugs.openjdk.java.net/browse/JDK-4358774 http://cr.openjdk.java.net/~bpb/4358774/webrev.00/ Add nullStream() method to each of InputStream and OutputStream. Thanks, Brian