RFR 4358774: Add null InputStream and OutputStream

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

Re: RFR 4358774: Add null InputStream and OutputStream

Alan Bateman


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 approach in 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 b.length is used on the next line. One other nit
is that the "overridden for efficiency" comment between @Override and
the method declaration is a distraction. I don't think the comment is
needed or just put it on the same line or above the @Overview so it
doesn't get in the way.

The NullXXX tests can be converted to TestNG unit tests if you have cycles.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
On Dec 8, 2017, at 4:39 AM, Alan Bateman <[hidden email]> 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 b.length is used on the next line.

Already made this change locally.

> One other nit is that the "overridden for efficiency" comment between @Override and the method declaration is a distraction. I don't think the comment is needed or just put it on the same line or above the @Overview so it doesn't get in the way.
>
> The NullXXX tests can be converted to TestNG unit tests if you have cycles.

I’ll make the above two changes and re-post later today.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
In reply to this post by Patrick Reinhart
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();
Writer.nullSink();

Another alternative would be simply to reflect the class names in the methods:

InputStream InputStream.nullInputStream();
OutputStream.nullOutputStream();

and later

Reader.nullReader();
Writer.nullWriter();

Comments?

Thanks,

Brian

On Dec 6, 2017, at 12:36 PM, Patrick Reinhart <[hidden email]> wrote:

> Is there also a issue for the same kind of methods for Reader and Writer?

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

David Lloyd-2
On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter
<[hidden email]> 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
>
> InputStream InputStream.nullSource();
> OutputStream.nullSink();
>
> and later
>
> Reader.nullSource();
> Writer.nullSink();
>
> Another alternative would be simply to reflect the class names in the methods:
>
> InputStream InputStream.nullInputStream();
> OutputStream.nullOutputStream();
>
> and later
>
> Reader.nullReader();
> Writer.nullWriter();

I for one prefer this alternative; it's very clear and unambiguous.

--
- DML
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
In reply to this post by Brian Burkhalter-2
On Dec 8, 2017, at 10:52 AM, David Lloyd <[hidden email]> wrote:

> On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter
> <[hidden email]> 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
>>
>> InputStream InputStream.nullSource();
>> OutputStream.nullSink();
>>
>> and later
>>
>> Reader.nullSource();
>> Writer.nullSink();
>>
>> Another alternative would be simply to reflect the class names in the methods:
>>
>> InputStream InputStream.nullInputStream();
>> OutputStream.nullOutputStream();
>>
>> and later
>>
>> Reader.nullReader();
>> Writer.nullWriter();
>
> I for one prefer this alternative; it's very clear and unambiguous.
The second one?

Thanks,

Brian

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Jonathan Bluett-Duncan
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 I'd find it a bit confusing to see `nullSource()` at first (especially
if it were statically-imported) as I would initially expect it to have a
return type of `*Source`.

Cheers,
Jonathan

On 8 December 2017 at 19:03, Brian Burkhalter <[hidden email]>
wrote:

> On Dec 8, 2017, at 10:52 AM, David Lloyd <[hidden email]> wrote:
>
> > On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter
> > <[hidden email]> 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
> >>
> >> InputStream InputStream.nullSource();
> >> OutputStream.nullSink();
> >>
> >> and later
> >>
> >> Reader.nullSource();
> >> Writer.nullSink();
> >>
> >> Another alternative would be simply to reflect the class names in the
> methods:
> >>
> >> InputStream InputStream.nullInputStream();
> >> OutputStream.nullOutputStream();
> >>
> >> and later
> >>
> >> Reader.nullReader();
> >> Writer.nullWriter();
> >
> > I for one prefer this alternative; it's very clear and unambiguous.
> The second one?
>
> Thanks,
>
> Brian
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

David Lloyd-2
In reply to this post by Brian Burkhalter-2
On Fri, Dec 8, 2017 at 1:03 PM, Brian Burkhalter
<[hidden email]> wrote:

> On Dec 8, 2017, at 10:52 AM, David Lloyd <[hidden email]> wrote:
>
>> On Fri, Dec 8, 2017 at 12:38 PM, Brian Burkhalter
>> <[hidden email]> 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
>>>
>>> InputStream InputStream.nullSource();
>>> OutputStream.nullSink();
>>>
>>> and later
>>>
>>> Reader.nullSource();
>>> Writer.nullSink();
>>>
>>> Another alternative would be simply to reflect the class names in the methods:
>>>
>>> InputStream InputStream.nullInputStream();
>>> OutputStream.nullOutputStream();
>>>
>>> and later
>>>
>>> Reader.nullReader();
>>> Writer.nullWriter();
>>
>> I for one prefer this alternative; it's very clear and unambiguous.
> The second one?

Yes, sorry.

--
- DML
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Patrick Reinhart
In reply to this post by Brian Burkhalter-2
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 <[hidden email]>:
>
> 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();
> Writer.nullSink();
>
> Another alternative would be simply to reflect the class names in the methods:
>
> InputStream InputStream.nullInputStream();
> OutputStream.nullOutputStream();
>
> and later
>
> Reader.nullReader();
> Writer.nullWriter();
>
> Comments?
>
> Thanks,
>
> Brian
>
> On Dec 6, 2017, at 12:36 PM, Patrick Reinhart <[hidden email]> wrote:
>
>> Is there also a issue for the same kind of methods for Reader and Writer?
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
In reply to this post by Brian Burkhalter-2
On Dec 8, 2017, at 8:29 AM, Brian Burkhalter <[hidden email]> wrote:

> On Dec 8, 2017, at 4:39 AM, Alan Bateman <[hidden email]> 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 b.length is used on the next line.
>
> Already made this change locally.
>
>> One other nit is that the "overridden for efficiency" comment between @Override and the method declaration is a distraction. I don't think the comment is needed or just put it on the same line or above the @Overview so it doesn't get in the way.
>>
>> The NullXXX tests can be converted to TestNG unit tests if you have cycles.
>
> I’ll make the above two changes and re-post later today.

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 consensus.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Sergey Bylokhov
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 as far as I understand it can return "0" if "len=0" even if
the stream was closed before:
   99             @Override
  100             public int read(byte[] b, int off, int len)
  101                 Objects.checkFromIndexSize(off, len, b.length);
  102                 if (len == 0) {
  103                     return 0;
  104                 }
  105                 ensureOpen();
  106                 return -1;
  107             }


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
Hi Sergey,

On Dec 8, 2017, at 4:34 PM, Sergey Bylokhov <[hidden email]> 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 the stream was closed before:
>  99             @Override
> 100             public int read(byte[] b, int off, int len)
> 101                 Objects.checkFromIndexSize(off, len, b.length);
> 102                 if (len == 0) {
> 103                     return 0;
> 104                 }
> 105                 ensureOpen();
> 106                 return -1;
> 107             }

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 until line 173.

Thanks,

Brian

[1] http://hg.openjdk.java.net/jdk/jdk/file/dd5157f363ab/src/java.base/share/classes/java/io/InputStream.java#l166
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brent Christian-2
In reply to this post by Brian Burkhalter-2
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 static void testreadNBytes() {
  111         try {
  112             assertEquals(0, openStream.readNBytes(new byte[1], 0, 1),
  113                 "readNBytes(byte[],int,int) != -1");

Line 113 should read "... != 0", yes?

  129     @Test(groups = "open")
  130     public static void testSkip() {
  131         try {
  132             assertEquals(0, openStream.skip(1), "transferTo() != 0");

On 132, it's "skip() != 0".

-Brent
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
In reply to this post by Brian Burkhalter-2
Hi Brent,

On Dec 8, 2017, at 4:50 PM, Brent Christian <[hidden email]> 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 are correct on both accounts. I shuffled that test completely around today converting it to TestNG and obviously made some typos. Webrev updated in place http://cr.openjdk.java.net/~bpb/4358774/webrev.03/.

Thanks,

Brian

Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Sergey Bylokhov
In reply to this post by Brian Burkhalter-2
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 until line 173.

Yes it is match the behavior, but both have a different specifications:
In the old methods there is a notion: "<p> If <code>len</code> is zero,
then no bytes are read and <code>0</code> is returned;" but in the new
method we have only one strong statement: "After the stream has been
closed, these methods all throw {@code IOException}."

--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
On Dec 8, 2017, at 5:06 PM, Sergey Bylokhov <[hidden email]> 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.) Note that the IOE for the stream being closed would not be thrown in the current code until line 173.
>
> Yes it is match the behavior, but both have a different specifications:
> In the old methods there is a notion: "<p> If <code>len</code> is zero, then no bytes are read and <code>0</code> is returned;" but in the new method we have only one strong statement: "After the stream has been closed, these methods all throw {@code IOException}."

Indeed you are correct. As we are trying to match the behavior I think the spec will need to be tweaked a little. The problem is that there is not a method-by-method specification in this case. This will need to be reexamined.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Patrick Reinhart
In reply to this post by Brian Burkhalter-2
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, b.length);

Should not be checked for a null byte buffer b here?

-Patrick
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Patrick Reinhart
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/
>>
>   99             @Override
>  100             public int read(byte[] b, int off, int len) throws IOException {
>  101                 Objects.checkFromIndexSize(off, len, b.length);
>
> Should not be checked for a null byte buffer b here?
>
> -Patrick


Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
In reply to this post by Brian Burkhalter-2

On Dec 8, 2017, at 3:12 PM, Brian Burkhalter <[hidden email]> 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 consensus.

As long as we’re still at it, given java.util.stream.Stream then

InputStream.nullInput()
OutputStream.nullOutput()
Reader.nullReader()
Writer.nullWriter()

might not be a bad alternative.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Brian Burkhalter-2
On Dec 11, 2017, at 12:52 PM, Brian Burkhalter <[hidden email]> wrote:

> On Dec 8, 2017, at 3:12 PM, Brian Burkhalter <[hidden email]> 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 consensus.
>
> As long as we’re still at it, given java.util.stream.Stream then
>
> InputStream.nullInput()
> OutputStream.nullOutput()
> Reader.nullReader()
> Writer.nullWriter()
>
> might not be a bad alternative.

Re-reading the discussion [1] of last year regarding [2], it seems that there was convergence on all points raised except the names of the new methods. The alternatives are:

A) InputStream.nullStream() and OutputStream.nullStream() as in the most recent version, webrev.03,
B) InputStream.nullInput() and OutputStream.nullOutput() as shown above, and
C) InputStream.nullInputStream() and OutputStream.nullOutputStream().

Does anyone have any more comments on this point? I would be inclined to choose option C.

Thanks,

Brian

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050367.html
[2] https://bugs.openjdk.java.net/browse/JDK-4358774
Reply | Threaded
Open this post in threaded view
|

Re: RFR 4358774: Add null InputStream and OutputStream

Chris Hegarty

> On 4 Jan 2018, at 23:42, Brian Burkhalter <[hidden email]> wrote:
>
>> On Dec 11, 2017, at 12:52 PM, Brian Burkhalter <[hidden email]> wrote:
>>
>>> On Dec 8, 2017, at 3:12 PM, Brian Burkhalter <[hidden email]> 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 consensus.
>>
>> As long as we’re still at it, given java.util.stream.Stream then
>>
>> InputStream.nullInput()
>> OutputStream.nullOutput()
>> Reader.nullReader()
>> Writer.nullWriter()
>>
>> might not be a bad alternative.
>
> Re-reading the discussion [1] of last year regarding [2], it seems that there was convergence on all points raised except the names of the new methods. The alternatives are:
>
> A) InputStream.nullStream() and OutputStream.nullStream() as in the most recent version, webrev.03,
> B) InputStream.nullInput() and OutputStream.nullOutput() as shown above, and
> C) InputStream.nullInputStream() and OutputStream.nullOutputStream().
>
> Does anyone have any more comments on this point? I would be inclined to choose option C.

After re-reading the email thread, I would choose option C too. It is unambiguous, and the ‘null{classname}’ pattern can be used for Reader/Writer in the future.

-Chris


> Thanks,
>
> Brian
>
> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050367.html
> [2] https://bugs.openjdk.java.net/browse/JDK-4358774

123