[Seaside-dev] Grease on Pharo7: streams

[Johan Brichau]

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 <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 <pdebruic at gmail.com> 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 <https://travis-ci.org/SeasideSt/Grease/jobs/406766720>
>> <https://travis-ci.org/SeasideSt/Grease/jobs/406766720&gt <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
>> <https://github.com/pharo-project/pharo/commit/9e442addff3790af0c78dbc73f1f349de8a4ad03#diff-ba7cc7962a4a22468a18b31fe75ada04&gt <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 at .squeakfoundation
> 
>> http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev <http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev>
> 
> 
> 
> 
> 
> --
> Sent from: http://forum.world.st/Seaside-Development-f1294793.html <http://forum.world.st/Seaside-Development-f1294793.html>
> _______________________________________________
> seaside-dev mailing list
> seaside-dev at lists.squeakfoundation.org <mailto:seaside-dev at lists.squeakfoundation.org>
> http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev <http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/seaside-dev/attachments/20180723/35dcee63/attachment-0001.html>