Copy for SequenceableCollection subclasses? (Was: Fix for 987)

> Thanks we will integrate that as soon as we can breath :)
>
> On Oct 5, 2009, at 12:14 PM, Henrik Johansen wrote:
>
>> Updated Issue987 with a SLICE in Inbox, which "fixes" the DNU in the
>> way below.
>> Used printString instead of asString as proposed by Hernán, as it (in
>> principle) should be more robustly implemented and thus more suited
>> for debugger display.
>> (If sig's idea from some time back was adopted, this should be
>> debugString instead of printString, of course).
>>
>> Cheers,
>> Henry
>>
>> On Oct 5, 2009, at 11:51 23AM, Henrik Johansen wrote:

> Somewhat related, at least I noticed it when debugging this:
>
> Sending copy to a LinkedList or a Semaphore will give back an array,
> since it uses shallowCopy from SequenceableCollection.
> Probably applies to more of SequenceableCollection's subclasses too,
> f.ex. DirectoryEntry for which copy leads to an MNU.
>
> Another side-effect of strange class-hierarchy, I guess :)
>
> Not quite sure how/if you want to implement those differently.
>
> Submitted to tracker as Issue1272.
>
> Cheers,
> Henry
>
>
> On Oct 5, 2009, at 1:12 38PM, Stéphane Ducasse wrote:
>
>> Thanks we will integrate that as soon as we can breath :)
>>
>> On Oct 5, 2009, at 12:14 PM, Henrik Johansen wrote:
>>
>>> Updated Issue987 with a SLICE in Inbox, which "fixes" the DNU in the
>>> way below.
>>> Used printString instead of asString as proposed by Hernán, as it (in
>>> principle) should be more robustly implemented and thus more suited
>>> for debugger display.
>>> (If sig's idea from some time back was adopted, this should be
>>> debugString instead of printString, of course).
>>>
>>> Cheers,
>>> Henry
>>>
>>> On Oct 5, 2009, at 11:51 23AM, Henrik Johansen wrote:
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>

> He he, I just noticed this last evening, see
> http://bugs.squeak.org/view.php?id=7402
> http://bugs.squeak.org/view.php?id=7403
> http://bugs.squeak.org/view.php?id=7404
>
> I thus provided another solution for
> http://bugs.squeak.org/view.php?id=6535 (previous solution has been
> uploaded in pharo).
>
> Nicolas
>
> 2009/10/5 Henrik Johansen <[hidden email]>:
>> Somewhat related, at least I noticed it when debugging this:
>>
>> Sending copy to a LinkedList or a Semaphore will give back an array,
>> since it uses shallowCopy from SequenceableCollection.
>> Probably applies to more of SequenceableCollection's subclasses too,
>> f.ex. DirectoryEntry for which copy leads to an MNU.
>>
>> Another side-effect of strange class-hierarchy, I guess :)
>>
>> Not quite sure how/if you want to implement those differently.
>>
>> Submitted to tracker as Issue1272.
>>
>> Cheers,
>> Henry
>>
>>
>> On Oct 5, 2009, at 1:12 38PM, Stéphane Ducasse wrote:
>>
>>> Thanks we will integrate that as soon as we can breath :)
>>>
>>> On Oct 5, 2009, at 12:14 PM, Henrik Johansen wrote:
>>>
>>>> Updated Issue987 with a SLICE in Inbox, which "fixes" the DNU in  
>>>> the
>>>> way below.
>>>> Used printString instead of asString as proposed by Hernán, as it  
>>>> (in
>>>> principle) should be more robustly implemented and thus more suited
>>>> for debugger display.
>>>> (If sig's idea from some time back was adopted, this should be
>>>> debugString instead of printString, of course).
>>>>
>>>> Cheers,
>>>> Henry
>>>>
>>>> On Oct 5, 2009, at 11:51 23AM, Henrik Johansen wrote:
>>
>> _______________________________________________
>> Pharo-project mailing list
>> [hidden email]
>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>

> Cheers,
> Henry
>
>
> On Oct 5, 2009, at 2:25 12PM, Nicolas Cellier wrote:
>
>> He he, I just noticed this last evening, see
>> http://bugs.squeak.org/view.php?id=7402
>> http://bugs.squeak.org/view.php?id=7403
>> http://bugs.squeak.org/view.php?id=7404
>>
>> I thus provided another solution for
>> http://bugs.squeak.org/view.php?id=6535 (previous solution has been
>> uploaded in pharo).
>>
>> Nicolas
>>
>> 2009/10/5 Henrik Johansen <[hidden email]>:
>>> Somewhat related, at least I noticed it when debugging this:
>>>
>>> Sending copy to a LinkedList or a Semaphore will give back an array,
>>> since it uses shallowCopy from SequenceableCollection.
>>> Probably applies to more of SequenceableCollection's subclasses too,
>>> f.ex. DirectoryEntry for which copy leads to an MNU.
>>>
>>> Another side-effect of strange class-hierarchy, I guess :)
>>>
>>> Not quite sure how/if you want to implement those differently.
>>>
>>> Submitted to tracker as Issue1272.
>>>
>>> Cheers,
>>> Henry
>>>
>>>
>>> On Oct 5, 2009, at 1:12 38PM, Stéphane Ducasse wrote:
>>>
>>>> Thanks we will integrate that as soon as we can breath :)
>>>>
>>>> On Oct 5, 2009, at 12:14 PM, Henrik Johansen wrote:
>>>>
>>>>> Updated Issue987 with a SLICE in Inbox, which "fixes" the DNU in
>>>>> the
>>>>> way below.
>>>>> Used printString instead of asString as proposed by Hernán, as it
>>>>> (in
>>>>> principle) should be more robustly implemented and thus more suited
>>>>> for debugger display.
>>>>> (If sig's idea from some time back was adopted, this should be
>>>>> debugString instead of printString, of course).
>>>>>
>>>>> Cheers,
>>>>> Henry
>>>>>
>>>>> On Oct 5, 2009, at 11:51 23AM, Henrik Johansen wrote:
>>>
>>> _______________________________________________
>>> Pharo-project mailing list
>>> [hidden email]
>>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>>
>>
>> _______________________________________________
>> Pharo-project mailing list
>> [hidden email]
>> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>>
>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>

> 2009/10/5 Henrik Johansen <[hidden email]>:
>> Good to know I'm not the only one who found it strange :)
>>
>> BTW, I thought the default for Collections was to do a shallowCopy
>> with a copy operation?
>>
>
> I removed this default behaviour SequenceableCollection>>shallowCopy
> which used to use (self copyFrom: 1 to: self size).
> This beautifull hack was using self species, leading to this mess...
> And it forced me defining a #basicShallowCopy to work the hack around
> (which was the shortest but uggliest solution).
>
> The copy was deep enough so that modifying the copy would not modify
> the original, and this is what postCopy now does.
>
>> ll := LinkedList new.
>> ll add: Link new.
>> ll2 := ll copy.
>> ll add: Link new.
>>
>> inspecting ll2, it's firstLink does not have a nextLink, which  
>> implies
>> to me a deepCopy has occured.
>>
>
> I did not take time to write all tests yet, I will give it a try in  
> trunk.
>
>> Of course, it's arguable that that's better behaviour for  
>> LinkedLists,
>> seeing as otherwise ll2's lastLink would be invalid when you added to
>> ll.
>> At a glance, SparseLargeTable, Stack (Through LinkedList
>> implementation) and WideCharacterSet in your changeset all seem to do
>> a deep copy.
>>
>
> just deep enough, not more, not less.

>
>> Also, how does part in SharedQueue2
>> monitor critical: [
>> monitor := Monitor new.
>> items := items copy
>> ]  work? Seems a bit fishy to me to have a critical section on a
>> monitor, then replacing it as part of it...
>>
>
> He he, it should just work, because we are talking to a Monitor object
> (the Smalltalk way), not to a pointer *monitor somewhere in memory
> (the C/C++ view).

> Thanks we will integrate that as soon as we can breath :)
>
> On Oct 5, 2009, at 12:14 PM, Henrik Johansen wrote:
>
>> Updated Issue987 with a SLICE in Inbox, which "fixes" the DNU in the
>> way below.
>> Used printString instead of asString as proposed by Hernán, as it (in
>> principle) should be more robustly implemented and thus more suited
>> for debugger display.
>> (If sig's idea from some time back was adopted, this should be
>> debugString instead of printString, of course).
>>
>> Cheers,
>> Henry
>>
>> On Oct 5, 2009, at 11:51 23AM, Henrik Johansen wrote:

>
> On Oct 5, 2009, at 3:19 53PM, Nicolas Cellier wrote:
>
>> 2009/10/5 Henrik Johansen <[hidden email]>:
>>> Good to know I'm not the only one who found it strange :)
>>>
>>> BTW, I thought the default for Collections was to do a shallowCopy
>>> with a copy operation?
>>>
>>
>> I removed this default behaviour SequenceableCollection>>shallowCopy
>> which used to use (self copyFrom: 1 to: self size).
>> This beautifull hack was using self species, leading to this mess...
>> And it forced me defining a #basicShallowCopy to work the hack around
>> (which was the shortest but uggliest solution).
>>
>> The copy was deep enough so that modifying the copy would not modify
>> the original, and this is what postCopy now does.
>>
>>> ll := LinkedList new.
>>> ll add: Link new.
>>> ll2 := ll copy.
>>> ll add: Link new.
>>>
>>> inspecting ll2, it's firstLink does not have a nextLink, which
>>> implies
>>> to me a deepCopy has occured.
>>>
>>

>> I did not take time to write all tests yet, I will give it a try in
>> trunk.
>>
>>> Of course, it's arguable that that's better behaviour for
>>> LinkedLists,
>>> seeing as otherwise ll2's lastLink would be invalid when you added to
>>> ll.
>>> At a glance, SparseLargeTable, Stack (Through LinkedList
>>> implementation) and WideCharacterSet in your changeset all seem to do
>>> a deep copy.
>>>
>>
>> just deep enough, not more, not less.
>
> To my understanding, that's too deep.
> You're copying elements in the collection as well, not just creating a
> copy of the collection with the same elements.
> That's what I'd define as a deepCopy, after a copy, I'd expect the
> elements in them to be the same.
> Maybe it's just what I've gotten used to in VW, and Squeak does
> something else historically, but mixing and matching which to do based
> on what feels best at any given time can lead to some nasty bugs :)
>

>>
>>> Also, how does part in SharedQueue2
>>> monitor critical: [
>>> monitor := Monitor new.
>>> items := items copy
>>> ]  work? Seems a bit fishy to me to have a critical section on a
>>> monitor, then replacing it as part of it...
>>>
>>
>> He he, it should just work, because we are talking to a Monitor object
>> (the Smalltalk way), not to a pointer *monitor somewhere in memory
>> (the C/C++ view).
>
> Oh, absolutely, it will not crash due to deallocated pointers or such.
>
> What I meant was a concurrency issue, when some other object requests
> something that requires a monitor lock, after Monitor has been
> replaced, but before the items have been copied.
> i.e.
> while the intent of the critical section in that piece of code as far
> as I can tell, is that the two operations should happen without items
> being accessible before they have been copied.
> so you'd actually have to write something like:
> monitor critical: [
>        monitor := Monitor new.
>        monitor critical:
>        [items := items copy]]
> to ensure that does not happen.
>