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? 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. 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. 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... 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 <henrik.s.johansen@veloxit.no>:

...

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 Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

On Oct 5, 2009, at 3:19 53PM, Nicolas Cellier wrote:

2009/10/5 Henrik Johansen <henrik.s.johansen@veloxit.no>:

...

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. Cheers, Henry

2009/10/5 Henrik Johansen <henrik.s.johansen@veloxit.no>:

On Oct 5, 2009, at 3:19 53PM, Nicolas Cellier wrote:

...

2009/10/5 Henrik Johansen <henrik.s.johansen@veloxit.no>:

...

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.

| list link1 link2 link3 copy | link1 := StackLink with: 1.0. link2 := StackLink with: 2.0. link3 := StackLink with: 3.0. link4 := StackLink with: 4.0. (list := LinkedList new) add: link1; add: link2; add: link3. copy := list copy. self assert: list size = 3. self assert: copy size = 3. self assert: copy first element == list first element. list remove: link2. self assert: list size = 2. self assert: copy size = 3. copy add: link4 before: copy third. self assert: list size = 2. self assert: copy size = 4. ^copy As you can see, this is not a deepCopy, the element identity are preserved. Of course not the link objects themselves, that can't be ! Otherwise modifying the copy would modify the original. Like Associations in Dictionary, they should be copied (but the value identity should not change).

...

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 :)

I don't think so, at least not in trunk image. The best thing would be to provide tests.

...

...

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.

IMO, this is currently unnecessary because no one yet knows about this copy. The sole use is ^self shallowCopy postCopy, so there is no pointer on me yet but the active process stack. We only have to protect against a modification of original in between But in case of exotic use of #postCopy, you are right. Just reversing the order would do. monitor critical: [ items := items copy monitor := Monitor new]. Nicolas

Cheers, Henry

_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project