Re: RFR 8196298 Add null Reader and Writer with latest changes

[Roger Riggs]

Hi Patrick,

Committed;  Thanks for the initiative and the perseverance.

Roger

On 3/18/2018 6:41 AM, Patrick Reinhart wrote:

Hi Roger,

Seems there are no more comments...   - all shocked :-)

-Patrick

Re: RFR 8196298 Add null Reader and Writer with latest changes

[Patrick Reinhart]

Hi Roger,

Seems there are no more comments...   - all shocked :-)

-Patrick

Am 16.03.2018 um 16:21 schrieb Roger Riggs:
> Hi Patrick,
>
> Looks good,
>
> If there are no more comments, I can sponsor the commit.
>
> Thanks, Roger
>
>
> On 3/16/18 2:10 AM, Patrick Reinhart wrote:
>> Just coming back on my webrev [1]. Are there any more feedbacks
>> implementation wise to that latest version? If no I would need a
>> commit sponsor for this change as the CSR [2] is reviewed and closed.
>>
>> -Patrick
>>
>> [1] http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.02
>> [2] https://bugs.openjdk.java.net/browse/JDK-8196350
>>
>>
>>> Am 07.03.2018 um 11:41 schrieb Patrick Reinhart :
>>>
>>> I applied those changes here:
>>>
>>> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.02
>>>
>>> -Patrick
>>>
>>>
 Am 06.03.2018 um 23:12 schrieb Brian Burkhalter
 :

 Yes, I think so and and also the parameter ‘csq' is allowed to be
 null so Objects.requireNonNull(csq) should be removed at lines 100
 and 107 as no NPE is specified for these methods [1, 2].

 Thanks,

 Brian

 [1]
 https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence)
 [2]
 https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence,int,int)

 On Mar 6, 2018, at 11:49 AM, Bernd Eckenfels
  wrote:

> Just a nit, Should append(CharSequence,int,int) also use
> checkFromIndexSize?
>

Re: RFR 8196298 Add null Reader and Writer with latest changes

[Roger Riggs]

Hi Patrick,

Looks good,

If there are no more comments, I can sponsor the commit.

Thanks, Roger


On 3/16/18 2:10 AM, Patrick Reinhart wrote:

Just coming back on my webrev [1]. Are there any more feedbacks implementation 
wise to that latest version? If no I would need a commit sponsor for this 
change as the CSR [2] is reviewed and closed.

-Patrick

[1] http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.02
[2] https://bugs.openjdk.java.net/browse/JDK-8196350



Am 07.03.2018 um 11:41 schrieb Patrick Reinhart :

I applied those changes here:

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.02

-Patrick



Am 06.03.2018 um 23:12 schrieb Brian Burkhalter :

Yes, I think so and and also the parameter ‘csq' is allowed to be null so 
Objects.requireNonNull(csq) should be removed at lines 100 and 107 as no NPE is 
specified for these methods [1, 2].

Thanks,

Brian

[1] 
https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence)
[2] 
https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence,int,int)

On Mar 6, 2018, at 11:49 AM, Bernd Eckenfels  wrote:


Just a nit, Should append(CharSequence,int,int) also use checkFromIndexSize?

RFR 8196298 Add null Reader and Writer with latest changes

[Patrick Reinhart]

Just coming back on my webrev [1]. Are there any more feedbacks implementation 
wise to that latest version? If no I would need a commit sponsor for this 
change as the CSR [2] is reviewed and closed.

-Patrick

[1] http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.02
[2] https://bugs.openjdk.java.net/browse/JDK-8196350


> Am 07.03.2018 um 11:41 schrieb Patrick Reinhart :
> 
> I applied those changes here:
> 
> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.02
> 
> -Patrick
> 
> 
>> Am 06.03.2018 um 23:12 schrieb Brian Burkhalter 
>> :
>> 
>> Yes, I think so and and also the parameter ‘csq' is allowed to be null so 
>> Objects.requireNonNull(csq) should be removed at lines 100 and 107 as no NPE 
>> is specified for these methods [1, 2].
>> 
>> Thanks,
>> 
>> Brian
>> 
>> [1] 
>> https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence)
>> [2] 
>> https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence,int,int)
>> 
>> On Mar 6, 2018, at 11:49 AM, Bernd Eckenfels  wrote:
>> 
>>> Just a nit, Should append(CharSequence,int,int) also use checkFromIndexSize?
>> 
> 

Re: RFR 8196298 Add null Reader and Writer with latest changes

[Patrick Reinhart]

I applied those changes here:

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.02

-Patrick


> Am 06.03.2018 um 23:12 schrieb Brian Burkhalter :
> 
> Yes, I think so and and also the parameter ‘csq' is allowed to be null so 
> Objects.requireNonNull(csq) should be removed at lines 100 and 107 as no NPE 
> is specified for these methods [1, 2].
> 
> Thanks,
> 
> Brian
> 
> [1] 
> https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence)
> [2] 
> https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence,int,int)
> 
> On Mar 6, 2018, at 11:49 AM, Bernd Eckenfels  wrote:
> 
>> Just a nit, Should append(CharSequence,int,int) also use checkFromIndexSize?
> 

Re: RFR 8196298 Add null Reader and Writer with latest changes

[Brian Burkhalter]

Yes, I think so and and also the parameter ‘csq' is allowed to be null so 
Objects.requireNonNull(csq) should be removed at lines 100 and 107 as no NPE is 
specified for these methods [1, 2].

Thanks,

Brian

[1] 
https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence)
[2] 
https://download.java.net/java/jdk10/docs/api/java/io/Writer.html#append(java.lang.CharSequence,int,int)

On Mar 6, 2018, at 11:49 AM, Bernd Eckenfels  wrote:

> Just a nit, Should append(CharSequence,int,int) also use checkFromIndexSize?

Re: RFR 8196298 Add null Reader and Writer with latest changes

[Bernd Eckenfels]

Just a nit, Should append(CharSequence,int,int) also use checkFromIndexSize?

Greetings
Bernd
-- 
http://bernd.eckenfels.net

Von: Alan Bateman
Gesendet: Dienstag, 6. März 2018 20:35
An: Patrick Reinhart; core-libs-dev
Betreff: Re: RFR 8196298 Add null Reader and Writer with latest changes

On 05/03/2018 21:45, Patrick Reinhart wrote:
> I tried to incorporate all feedback I received so far and updated the webrev 
> accordingly:
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01 
> <http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01>
>
I think this looks good now. I've added myself as a reviewer on the CSR 
so you should be able to finalize it.

-Alan

Re: RFR 8196298 Add null Reader and Writer with latest changes

[Patrick Reinhart]

Updated the CSR API with webrev's content and finalized it..

-Patrick

Am 06.03.2018 um 17:35 schrieb Alan Bateman:
> On 05/03/2018 21:45, Patrick Reinhart wrote:
>> I tried to incorporate all feedback I received so far and updated the
>> webrev accordingly:
>>
>> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01
>> 
>>
> I think this looks good now. I've added myself as a reviewer on the
> CSR so you should be able to finalize it.
>
> -Alan

Re: RFR 8196298 Add null Reader and Writer with latest changes

[Alan Bateman]

On 05/03/2018 21:45, Patrick Reinhart wrote:

I tried to incorporate all feedback I received so far and updated the webrev 
accordingly:

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01 


I think this looks good now. I've added myself as a reviewer on the CSR 
so you should be able to finalize it.


-Alan

RFR 8196298 Add null Reader and Writer with latest changes

[Patrick Reinhart]

I tried to incorporate all feedback I received so far and updated the webrev 
accordingly:

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01 


-Patrick

RFR 8196298 Add null Reader and Writer

[Patrick Reinhart]

Hi every boy that reviewed

I tried to incorporate all feedback I received so far and updated the webrev 
accordingly:

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01

-Patrick

Re: RFR 8196298 Add null Reader and Writer

[Patrick Reinhart]

Hi everyone responded so far.

How should we proceed now?

-Patrick

Am 07.02.2018 um 18:38 schrieb Patrick Reinhart:
>> Am 07.02.2018 um 14:03 schrieb Alan Bateman :
>>
>> On 03/02/2018 17:05, Patrick Reinhart wrote:
>>> :
>>> I reworked the tests and Writer implementation accordingly
>>>
>>> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01
>>>
>> Just catching up on this.
>>
>> nullReader’s javadoc suggests that mark(int) does not nothing but this seems 
>> to conflict with the Reader's javadoc where it is specified to throw IOE 
>> when mark is not supported.
> So I will change the API description accordingly there…
>
>> I'm a bit uneasy with len==0 check in the read method. I realize this is 
>> trying to mirror InputStream.read and maybe some Reader implementations but 
>> it's not specified behavior. I think we have to re-examine the Reader spec 
>> to clarify this to avoid creating more inconsistencies.
> That’s exactly what I did, so for me it’s ok to look into the Reader spec 
> also as I already modify that File. I personally thought that a null like 
> reader will always be at the end of the stream and therefore return -1 and do 
> no checking of how much space may be left on the consuming end. That seem to 
> me more the natural way of thinking.
>
>> Similarly in nullWriter() where it looks like the the len==0 checks is in 
>> unspecified territory. I think this will need clarifications to the Writer 
>> spec. One obvious inconsistency is to close the writer and call write("") 
>> and write("",  0, 0). The first will fail as the writer is closed, the 
>> second does not fail.
> Here I think the same holds true as I wrote for the Reader…
>
>> Another one is read(CharBuffer). Suppose CharBuffer cb has 0 bytes 
>> remaining; if I invoke nullReader().read(cb) then it looks like it will 
>> return 0 whereas the read(CharBuffer) method is specified to return -1.
>>
> I see your point there, the implementation I did there was taken from the 
> StringReader, where it will be the same len==0 check as you mentioned before… 
> So it looks like we really need to look into those implementations…
>
> -Patrick
>
>

Re: RFR 8196298 Add null Reader and Writer

[Patrick Reinhart]

> Am 07.02.2018 um 14:03 schrieb Alan Bateman :
> 
> On 03/02/2018 17:05, Patrick Reinhart wrote:
>> :
>> I reworked the tests and Writer implementation accordingly
>> 
>> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01
>> 
> Just catching up on this.
> 
> nullReader’s javadoc suggests that mark(int) does not nothing but this seems 
> to conflict with the Reader's javadoc where it is specified to throw IOE when 
> mark is not supported.

So I will change the API description accordingly there…

> 
> I'm a bit uneasy with len==0 check in the read method. I realize this is 
> trying to mirror InputStream.read and maybe some Reader implementations but 
> it's not specified behavior. I think we have to re-examine the Reader spec to 
> clarify this to avoid creating more inconsistencies.

That’s exactly what I did, so for me it’s ok to look into the Reader spec also 
as I already modify that File. I personally thought that a null like reader 
will always be at the end of the stream and therefore return -1 and do no 
checking of how much space may be left on the consuming end. That seem to me 
more the natural way of thinking.

> 
> Similarly in nullWriter() where it looks like the the len==0 checks is in 
> unspecified territory. I think this will need clarifications to the Writer 
> spec. One obvious inconsistency is to close the writer and call write("") and 
> write("",  0, 0). The first will fail as the writer is closed, the second 
> does not fail.

Here I think the same holds true as I wrote for the Reader…

> 
> Another one is read(CharBuffer). Suppose CharBuffer cb has 0 bytes remaining; 
> if I invoke nullReader().read(cb) then it looks like it will return 0 whereas 
> the read(CharBuffer) method is specified to return -1.
> 

I see your point there, the implementation I did there was taken from the 
StringReader, where it will be the same len==0 check as you mentioned before… 
So it looks like we really need to look into those implementations…

-Patrick

Re: RFR 8196298 Add null Reader and Writer

[Alan Bateman]

On 03/02/2018 17:05, Patrick Reinhart wrote:

:
I reworked the tests and Writer implementation accordingly

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01


Just catching up on this.

nullReader's javadoc suggests that mark(int) does not nothing but this 
seems to conflict with the Reader's javadoc where it is specified to 
throw IOE when mark is not supported.


I'm a bit uneasy with len==0 check in the read method. I realize this is 
trying to mirror InputStream.read and maybe some Reader implementations 
but it's not specified behavior. I think we have to re-examine the 
Reader spec to clarify this to avoid creating more inconsistencies.


Similarly in nullWriter() where it looks like the the len==0 checks is 
in unspecified territory. I think this will need clarifications to the 
Writer spec. One obvious inconsistency is to close the writer and call 
write("") and write("",  0, 0). The first will fail as the writer is 
closed, the second does not fail.


Another one is read(CharBuffer). Suppose CharBuffer cb has 0 bytes 
remaining; if I invoke nullReader().read(cb) then it looks like it will 
return 0 whereas the read(CharBuffer) method is specified to return -1.


-Alan

Re: RFR 8196298 Add null Reader and Writer

[Patrick Reinhart]

> Am 02.02.2018 um 02:45 schrieb Stuart Marks :
> 
> 
> 
> On 2/1/18 10:35 AM, Patrick Reinhart wrote:
>> Here is my first shot of the actual implementation and tests:
>> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.00
> 
> I looked at the specification of ready() vs the implementation. The spec says 
> that ready(), among other methods, behaves as if the Reader is at EOF. The 
> spec for Reader.ready() says "true if the next read() is guaranteed not to 
> block for input." Thus I'd assume that at EOF, read() will return -1 
> immediately, without blocking, so ready() should return true.
> 
> The NullReader implementation inherits ready() from Reader, which always 
> returns false.
> 
> So, the behavior doesn't seem consistent with my reading of the specs. 
> However, other implementations aren't, either. For example, the spec for 
> CharArrayReader.ready() says "Character-array readers are always ready to be 
> read." However,
> 
>new CharArrayReader(new char[0]).ready()
> 
> returns false!
> 
> A couple other readers I tried (InputStreamReader, FileReader) also have 
> ready() return true until they reach EOF, at which time they return false. 
> But StringReader.ready() always returns true. H.
> 
> Maybe the safest thing to do is to leave the various Readers' behavior of 
> ready() as they are, and leave nullReader's behavior to be consistent with 
> them. No changes are necessary to Reader in this webrev.
> 
> We should probably take a pass over the ready() methods in the Reader family 
> to see if they're correct. I'd suggest they need adjustment to mean that the 
> next read() won't block *and* data is available. And the 
> CharArrayReader.ready() spec should be fixed.
> 
> **
> 
> On the Writer side, the spec should mention that the three append() methods 
> and five write() methods do nothing if open and throw IOE if the Writer is 
> closed.
> 
> The Writer.flush() method spec doesn't say what happens if flush() is called 
> after the Writer is closed. However, Writer.close() says that subsequent 
> write() or flush() calls throw IOE. Writer implementations that track 
> open/closed state, such as FileWriter, will throw IOE from flush() after 
> closure. Since it's also tracking open/closed state, I'd expect 
> nullWriter.flush() to do the same. That is, do nothing if open, throw IOE if 
> closed.
> 
> Tests for flush() should be added.
> 

I stated in the nullWriter() method that flush() will have no action. I 
certainly can implement that behavior, so in consequence I will change the API 
description of the nullWriter() method accordingly.


> **
> 
> Speaking of tests, the tests for proper exception-throwing for the closed 
> cases can probably usefully employ the
> 
> @Test(expectedExceptions=IOException.class)
> 
> annotation. The reason I often avoid this construct is that if the same 
> exception class is thrown from an unexpected part of the test, it can mask 
> actual failures. However, since these are one-liners, that reason doesn't 
> apply. Thus:
> 
>@Test(groups="closed", expectedExceptions=IOException.class)
>public static void testAppendCharClosed() {
>closedWriter.append('x');
>}
> 
> You can probably also omit the try/catch block from the open tests, so that 
> instead of
> 
>@Test(groups="open")
>public static void testAppendChar() {
>try {
>assertSame(openWriter, openWriter.append('x'));
>} catch (IOException e) {
>fail("Unexpected IOException");
>}
>}
> 
> you can write
> 
>@Test(groups="open")
>public static void testAppendChar() {
>assertSame(openWriter, openWriter.append('x'));
>}
> 
> since the framework should catch and report errant exceptions, without the 
> need for code to catch exceptions and call fail().
> 
> s’marks
> 

I reworked the tests and Writer implementation accordingly

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01

-Patrick

Re: RFR 8196298 Add null Reader and Writer

[Stuart Marks]

On 2/1/18 10:35 AM, Patrick Reinhart wrote:

Here is my first shot of the actual implementation and tests:

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.00


I looked at the specification of ready() vs the implementation. The spec says 
that ready(), among other methods, behaves as if the Reader is at EOF. The spec 
for Reader.ready() says "true if the next read() is guaranteed not to block for 
input." Thus I'd assume that at EOF, read() will return -1 immediately, without 
blocking, so ready() should return true.


The NullReader implementation inherits ready() from Reader, which always returns 
false.


So, the behavior doesn't seem consistent with my reading of the specs. However, 
other implementations aren't, either. For example, the spec for 
CharArrayReader.ready() says "Character-array readers are always ready to be 
read." However,


new CharArrayReader(new char[0]).ready()

returns false!

A couple other readers I tried (InputStreamReader, FileReader) also have ready() 
return true until they reach EOF, at which time they return false. But 
StringReader.ready() always returns true. H.


Maybe the safest thing to do is to leave the various Readers' behavior of 
ready() as they are, and leave nullReader's behavior to be consistent with them. 
No changes are necessary to Reader in this webrev.


We should probably take a pass over the ready() methods in the Reader family to 
see if they're correct. I'd suggest they need adjustment to mean that the next 
read() won't block *and* data is available. And the CharArrayReader.ready() spec 
should be fixed.


**

On the Writer side, the spec should mention that the three append() methods and 
five write() methods do nothing if open and throw IOE if the Writer is closed.


The Writer.flush() method spec doesn't say what happens if flush() is called 
after the Writer is closed. However, Writer.close() says that subsequent write() 
or flush() calls throw IOE. Writer implementations that track open/closed state, 
such as FileWriter, will throw IOE from flush() after closure. Since it's also 
tracking open/closed state, I'd expect nullWriter.flush() to do the same. That 
is, do nothing if open, throw IOE if closed.


Tests for flush() should be added.

**

Speaking of tests, the tests for proper exception-throwing for the closed cases 
can probably usefully employ the


@Test(expectedExceptions=IOException.class)

annotation. The reason I often avoid this construct is that if the same 
exception class is thrown from an unexpected part of the test, it can mask 
actual failures. However, since these are one-liners, that reason doesn't apply. 
Thus:


@Test(groups="closed", expectedExceptions=IOException.class)
public static void testAppendCharClosed() {
closedWriter.append('x');
}

You can probably also omit the try/catch block from the open tests, so that 
instead of


@Test(groups="open")
public static void testAppendChar() {
try {
assertSame(openWriter, openWriter.append('x'));
} catch (IOException e) {
fail("Unexpected IOException");
}
}

you can write

@Test(groups="open")
public static void testAppendChar() {
assertSame(openWriter, openWriter.append('x'));
}

since the framework should catch and report errant exceptions, without the need 
for code to catch exceptions and call fail().


s'marks

Re: RFR 8196298 Add null Reader and Writer

[Brian Burkhalter]

On Feb 1, 2018, at 12:01 PM, Patrick Reinhart  wrote:

>> I believe read(CharBuffer), like read(char[], int, int), should test if 
>> there are remaining chars in the buffer.
> 
> Does it make sense to check for remaining chars in the buffer, wen the source 
> is basically at the end of it’s data?

read(char[], int, int) returns 0 if len == 0 and -1 otherwise.

read(CharBuffer) returns 0 if hasRemaining() is false and -1 otherwise.

The two behaviors seem consistent.

Brian

Re: RFR 8196298 Add null Reader and Writer

[Patrick Reinhart]

Hi Rémi,

I also found, that I missed the throw an ReadOnlyBufferException on a passed in 
read only CharBuffer.

> Am 01.02.2018 um 19:49 schrieb Remi Forax :
> 
> Hi Patrick,
> I believe read(CharBuffer), like read(char[], int, int), should test if there 
> are remaining chars in the buffer.

Does it make sense to check for remaining chars in the buffer, wen the source 
is basically at the end of it’s data?

-Patrick

Re: RFR 8196298 Add null Reader and Writer

[Remi Forax]

Hi Patrick,
I believe read(CharBuffer), like read(char[], int, int), should test if there 
are remaining chars in the buffer.

cheers,
Rémi

- Mail original -
> De: "Patrick Reinhart" <patr...@reini.net>
> À: "core-libs-dev" <core-libs-dev@openjdk.java.net>
> Envoyé: Jeudi 1 Février 2018 19:35:11
> Objet: RFR 8196298 Add null Reader and Writer

> Hi,
> 
> Here is my first shot of the actual implementation and tests:
> 
> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.00
> 
> Thanks for feedback
> 
> -Patrick

RFR 8196298 Add null Reader and Writer

[Patrick Reinhart]

Hi,

Here is my first shot of the actual implementation and tests:

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.00

Thanks for feedback

-Patrick