Issue 4596 in pharo: WeakOrderedColllection array is replaced by an Array instead of a WeakArray

Status: Accepted
Owner: [hidden email]

New issue 4596 by [hidden email]: WeakOrderedColllection array is  
replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

If an Array is used, then the references are no more weak and  
WeakorderedCollection is then broken.

This is because each reference to Array should be replaced with (self  
arrayType) from within OrderedCollection.

Typically, OrderedCollection>>#grow is such an operation breaking the  
WeakArray.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #1 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

See SLICE in inbox for patch.
Mariano, you shall add a test...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #2 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Hmm the "atomic" MCPackageLoader is once more messing up, removing #grow  
BEFORE modifying its usage...

You'll need to split the SLICE in two parts, firt apply method changes,  
second apply removal of grow.

Also we might want to use self growSize in growAtFirst, growAtLast...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #3 on issue 4596 by marianopeck: WeakOrderedColllection array is  
replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Ok, I have splitted the slice in two parts: PART1 and PART2. PART1 includes  
the fixes in #makeRoomAtFirst and #makeRoomAtLast and a new class  
WeakOrderedCollectionTest  with two tests:  
#testWeakOrderedCollectionSomeGarbageCollected and  
#testWeakOrderedCollectionAllGarbageCollected.

Those tests can be improved a lot and make them look like those in  
WeakSetTest where each collection operation is tested. I don't have the  
time/knowledge to do that right now, but if someone can, excellent.

The second part is just the revome of OrderedCollection >> grow.


Name:  
SLICE-Issue-4596-WeakOrderedColllection-array-is-replaced-by-an-Array-instead-of-a-WeakArray-PART1-MarianoMartinezPeck.1
Author: MarianoMartinezPeck
Time: 3 August 2011, 2:40:10 pm
UUID: 35f20200-98ef-581f-1100-000044ef581f
Ancestors:
Dependencies: CollectionsTests-MarianoMartinezPeck.529,  
Collections-Sequenceable-MarianoMartinezPeck.97

This is the first part. It includes the fix and 2 tests.




Name:  
SLICE-Issue-4596-WeakOrderedColllection-array-is-replaced-by-an-Array-instead-of-a-WeakArray-PART2-MarianoMartinezPeck.1
Author: MarianoMartinezPeck
Time: 3 August 2011, 2:41:42 pm
UUID: caf00200-1007-401f-18ec-0e0008e05c1f
Ancestors:
Dependencies: Collections-Sequenceable-MarianoMartinezPeck.98

It just removes OrderedCollection >> grow



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixProposed

Comment #4 on issue 4596 by marianopeck: WeakOrderedColllection array is  
replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: Milestone-1.4

Comment #5 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #6 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Thanks a lot.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #7 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

To be turned into a test


object := Object new.

coll := WeakOrderedCollection new add: anObject.
anObject := nil.
Smalltalk garbageCollect.
self assert: coll removeLast isNil



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #8 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

That was not enought in this case. In fact, it was really difficult for me  
to find the bug, because it used to ONLY fail when sending #grow.

(WeakOrderedCollection new instVarNamed: 'array') class ->>>> WeakArray


(WeakOrderedCollection new grow instVarNamed: 'array') class  ->>>> Array



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #9 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Do we add this to 1.3, too?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #10 on issue 4596 by marianopeck: WeakOrderedColllection array is  
replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Marcus, if you want to add it, it would be nice because right now  
WeakOrderedCollection is broken, but there is no user in the core, and me,  
the only user so far, can live by just doing the change in my image ;)

So....I would let it for 1.4 since it is not a CRITICAL issue.

BTW, can we release 1.3 already???  you spend too much time integrating  
stuff in both places...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #11 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Ok, than let's release 1.3 this week.

There are no issues marked 1.3 and it was used quite a lot.




_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #12 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Yes, but it is probably used by Magma or other tools.
Not being used in core is not a valid criterion, the core in itself has no  
value, the applications have.
Mariano, you must change thinking and realize you're proposing a service to  
customers. Everything left in the image is such a service.

It is better to remove non-functionning classes from core and let thrid  
party deal with it rather than letting them rot in image.
Though, such removals put the burden of Metacello configuration  
complexifications on customer shoulders, so if an easy fix is available,  
solution #1 is to apply it and include the test.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: Milestone-1.3

Comment #13 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #14 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

Of course, merging that in 1.3 is not possible as it was done for 1.4.

So I will add it to 1.4 first, than we need to find a solution for 1.3.

Sometimes I think: Changesets are not that bad...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #15 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

1.4 is DONE (14075 and 14076)

Now we need to do it again on top of 1.3


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #16 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

And we should do it fast... I normally wanted to build the final 1.3 images  
today.



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixToInclude

Comment #17 on issue 4596 by marianopeck: WeakOrderedColllection array is  
replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #18 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

... working on it now


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #19 on issue 4596 by [hidden email]: WeakOrderedColllection  
array is replaced by an Array instead of a WeakArray
http://code.google.com/p/pharo/issues/detail?id=4596

So now I wonder: Why is removing #grow the way to go?

We can not just remove a public method of OrderedCollection in the very  
very last update to 1.3!!!!

At the beginning of the report I read:

"This is because each reference to Array should be replaced with (self  
arrayType) from within OrderedCollection."

Why was this not done?




_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker