Grease on Pharo7: streams

On 23 Jul 2018, at 20:33, Paul DeBruicker <[hidden email]> wrote:

Hi Johan,


I just downloaded Pharo 7 (on the mac, 64bit image/vm) to try this out
because I was wondering if it was filling with spaces or null characters and
inspecting


(ReadWriteStream on: (String new: 4096)) contents


gave an empty string.  Could something else cause the 4096 char string? 






Johan Brichau-2 wrote

Hi all,

There have been a number of changes to the ReadWriteStream implementation
in Pharo 7 that break Grease stream tests:
https://travis-ci.org/SeasideSt/Grease/jobs/406766720
&lt;https://travis-ci.org/SeasideSt/Grease/jobs/406766720&gt;
I think there is both an issue in Grease for Pharo and in Pharo 7 itself.

* The most important issue is that the following expression returns a
different result on Pharo 7 when compared with earlier versions:

(ReadWriteStream on: (String new: 4096)) contents

On Pharo 6, this returns an empty string.
On Pharo 7, this returns a string with 4096 white space chars.

I’m inclined to say that Pharo 7 behavior is correct.
I read through the ANSI document and `contents` should return the future
sequences as well… which effectively means returning all those empty
chars.
I’m a bit in doubt why this has been different all that time… hence my
email.

The core of the issue is that GRPharoPlatform>>readWriteCharacterStream is
`ReadWriteStream on: (String new: 4096)`, hence a String instance with a
lot of spaces ;)
In GRGemStonePlatform, the readWriteStream is initialized on an empty
string.
In GRSqueakPlatform, the RWBinaryOrTextStream class is used on ‘ByteArray
new: 4096’.

Changing the implementation of GRPharoPlatform>>readWriteCharacterStream
to `ReadWriteStream on: String new` fixes most of the tests.

* The next issue is that `ReadWriteStream>>contents` has been changed in
https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04
&lt;https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04&gt;
This means that:

| stream |
stream := GRPlatform current readWriteCharacterStream.
stream
nextPutAll: 'abc';
reset.
self assert: stream contents = 'abc’.

This is no longer true in Pharo 7.
ANSI docs says that `reset` should only reset the pointer position and
thus the future sequences on the stream should still be returned after
resetting. 
Reverting the change in the commit, also fixes that test.
It’s unclear why that change was made though, so I’ll try to talk to
someone at Pharo.

Would be nice to hear other’s thoughts.

Cheers
Johan
_______________________________________________
seaside-dev mailing list

seaside-dev@.squeakfoundation

http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev

--
Sent from: http://forum.world.st/Seaside-Development-f1294793.html
_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev

On 24 Jul 2018, at 10:18, Johan Brichau <[hidden email]> wrote:

So, after digging around, I have corrected my report on the broken ReadWriteStream behavior.

The problem is described here: https://pharo.fogbugz.com/f/cases/22281/ReadWriteStream-broken

I’ll make a PR for Pharo and let’s see where it goes.

Johan

On 23 Jul 2018, at 21:03, Johan Brichau <[hidden email]> wrote:

Hi Paul,

I’ve been somewhat afloat in my mind trying to figure out what was wrong.

Unfortunately, I think that `(ReadWriteStream on: (String new: 4096)) contents` returning a string with null chars was due to my own changes trying to fix the tests.

My latest changes make fixes to Pharo to get back to the desired behavior. (https://github.com/SeasideSt/Grease/commits/pharo7)

They actually revert 2 changes in Pharo 7 and I do not see why they were done.

There are also very few tests for streams in Pharo itself. I’m trying to make a PR to fix that.

I must correct in that (ReadWriteStream on: 'abc') contents should indeed return an empty string, as pointed out by Richard on the mailinglist in http://lists.pharo.org/pipermail/pharo-users_lists.pharo.org/2018-July/040162.html

In this respect, I discovered another change that introduced the ReadWriteStream>>on: method which, strangely, sets the readLimit position to the end of the stream. Removing this method (introduced in https://github.com/pharo-project/pharo/commit/41f3a88863f7d9b35509e6e43f7d3ac2df9801e4#diff-ba7cc7962a4a22468a18b31fe75ada04) and reverting the change in https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04 re-establishes the stream behavior that is tested by the stream tests in Grease.

I hope I’m on the right path this time ;)

cheers

Johan

On 23 Jul 2018, at 20:33, Paul DeBruicker <[hidden email]> wrote:

Hi Johan,


I just downloaded Pharo 7 (on the mac, 64bit image/vm) to try this out
because I was wondering if it was filling with spaces or null characters and
inspecting


(ReadWriteStream on: (String new: 4096)) contents


gave an empty string.  Could something else cause the 4096 char string? 






Johan Brichau-2 wrote

Hi all,

There have been a number of changes to the ReadWriteStream implementation
in Pharo 7 that break Grease stream tests:
https://travis-ci.org/SeasideSt/Grease/jobs/406766720
&lt;https://travis-ci.org/SeasideSt/Grease/jobs/406766720&gt;
I think there is both an issue in Grease for Pharo and in Pharo 7 itself.

* The most important issue is that the following expression returns a
different result on Pharo 7 when compared with earlier versions:

(ReadWriteStream on: (String new: 4096)) contents

On Pharo 6, this returns an empty string.
On Pharo 7, this returns a string with 4096 white space chars.

I’m inclined to say that Pharo 7 behavior is correct.
I read through the ANSI document and `contents` should return the future
sequences as well… which effectively means returning all those empty
chars.
I’m a bit in doubt why this has been different all that time… hence my
email.

The core of the issue is that GRPharoPlatform>>readWriteCharacterStream is
`ReadWriteStream on: (String new: 4096)`, hence a String instance with a
lot of spaces ;)
In GRGemStonePlatform, the readWriteStream is initialized on an empty
string.
In GRSqueakPlatform, the RWBinaryOrTextStream class is used on ‘ByteArray
new: 4096’.

Changing the implementation of GRPharoPlatform>>readWriteCharacterStream
to `ReadWriteStream on: String new` fixes most of the tests.

* The next issue is that `ReadWriteStream>>contents` has been changed in
https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04
&lt;https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04&gt;
This means that:

| stream |
stream := GRPlatform current readWriteCharacterStream.
stream
nextPutAll: 'abc';
reset.
self assert: stream contents = 'abc’.

This is no longer true in Pharo 7.
ANSI docs says that `reset` should only reset the pointer position and
thus the future sequences on the stream should still be returned after
resetting. 
Reverting the change in the commit, also fixes that test.
It’s unclear why that change was made though, so I’ll try to talk to
someone at Pharo.

Would be nice to hear other’s thoughts.

Cheers
Johan
_______________________________________________
seaside-dev mailing list

seaside-dev@.squeakfoundation

http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev

--
Sent from: http://forum.world.st/Seaside-Development-f1294793.html
_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev