On Tue, 6 Dec 2011, David T. Lewis wrote:

> On Tue, Dec 06, 2011 at 08:18:43AM -0300, Juan Vuletich wrote:
>> Hi Folks,
>>
>> Levente Uzonyi wrote:
>>> On Mon, 5 Dec 2011, David T. Lewis wrote:
>>>
>>>> On Mon, Dec 05, 2011 at 05:02:57PM +0100, Levente Uzonyi wrote:
>>>>> On Wed, 30 Nov 2011, David T. Lewis wrote:
>>>>>>
>>>>>> Given apparent agreement that this is the correct approach, and
>>>>>> considering
>>>>>> this as a bug fix (not new feature) for ReferenceStream, I have moved
>>>>>> Juan's changes to trunk.
>>>>>
>>>>> After these changes we have five new errors when running all tests:
>>>>>
>>>>> BitmapStreamTests>>#testMatrixTransform2x3WithImageSegment
>>>>> BitmapStreamTests>>#testShortIntegerArrayWithImageSegment
>>>>> BitmapStreamTests>>#testShortPointArrayWithImageSegment
>>>>> BitmapStreamTests>>#testShortRunArrayWithImageSegment
>>>>> BitmapStreamTests>>#testWordArrayWithImageSegment
>>>>>
>>>>> The problem seems to be with DiskProxy's serialization, because the
>>>>> cause
>>>>> of the error is that it's not found in the structures dictionary during
>>>>> materialization.
>>>>>
>>>>
>>>> Thanks for catching this.
>>>>
>>>> The problem is certainly in the serialization, because
>>>> #testMatrixTransform2x3WithImageSegment
>>>> should be writing a file called 'bitmapStreamTest.extSeg' of size
>>>> 1549 bytes,
>>>> but after the ReferenceStream changes the file is only 1380 bytes in
>>>> size,
>>>> indicating that something that should have been written to the file is
>>>> now missing. I'm not sure of the cause though.
>>>
>>> I found that the new implementation of ReferenceStream >> #references
>>> causes the problems. There are two different issues with it:
>>> 1) it doesn't return all non-weak references
>>> 2) it does return a copy
>>
>> Yes. You're right.
>>
>>> DiskProxy isn't serialized, because it's added to the references as
>>> DiskProxy -> #none, so it won't be returned by #references (1),
>>> therefore SmartRefStream >> #instVarInfo: won't add it to the structures.
>>>
>>> Other classes are not serialized, because (e.g.) ImageSegment >>
>>> #storeDataOn: tries to modify aDataStream's references. If that method
>>> returns a copy (2), then the modification won't have any effect.
>>>
>>> What can we do?
>>>
>>> To solve (1) we have to make sure that all non-weak references are
>>> returned. It seems to me that weak references have an
>>> OrderedCollection as their value in the dictionary, but I don't know
>>> what's the reason for that. Anyway, using [ :value | value class ~~
>>> OrderedCollection ] seems to be a possible solution. Another solution
>>> is to add a method that returns all references and use it from places
>>> where all of them is needed instead of #references.
>>
>> My first implementation of #references didn't include the special #none
>> symbol, and while it fixed ReferenceStream, it broke SmartRefStream. The
>> attach includes a new one that does include #none. It also includes this
>> hopefully useful comment:
>>    "Values can be:
>>        - integer: regular reference. Value is position in the output
>> stream.
>>        - #none: special value for DiskProxy. Value is just a marker.
>>        - A collection of (still) weak only references. They are turned
>> into regular references when the first regular reference to the key is
>> dumped.
>>    See #tryToPutReference:typeID:"
>>    "Warning: Methods that need to deal with the internals of dumping
>> weak references, or methods that need to actually modify the references
>> dictionary CAN NOT use this method!"
>>    "Do not include provisory references created in #nextPutWeak that
>> never became normal references,
>>    because the referenced object was never added from a call to #nextPut:"
>>
>>> To solve (2), the best thing to do is to add new methods to
>>> ReferenceStream, allowing indirect manipulation of the contents of
>>> it's references variable and use these methods from ImageSegment. This
>>> would be more explicit than using a method that returns the references
>>> dictionary.
>>
>> Yes. I had already done this in #addSpecialReference: .
>>
>> What is really needed here is to track all users of the 'references'
>> ivar and all senders of #references, and for each one, decide if it
>> needs (1) the copy without the weak refs, or (2) the full dictionary,
>> possibly to modify it. I did this for Cuis, and this is in my change
>> set, but in Squeak there are many more users of the ivar and senders of
>> the method, so more work is needed.
>>
>> I also included better tests, that catch the DiskProxy bug I introduced.
>>
>> Thanks for finding the bug, and for working on this.
>>
>> Cheers,
>> Juan Vuletich
>
> Thanks Juan and Levente,
>
> I tried adding these changes and it looks like there are a few issues
> still to be sorted out. I suspect we may want to just back these updates
> out of trunk until after the Squeak 4.3 release is complete, then put
> them back and finish any remaining work after the release is done.
>
> comments?

I tried to fix it last night, but ImageSegment breaks the unwritten 
contract of ReferenceStream, so I'll probably just restore the previous 
behavior today and we'll fix this issue after the release of 4.3.


Levente

>
> Dave
>
>
>