Mantis - Squeak
Viewing Issue Advanced Details
6937 Collections minor always 02-19-08 18:12 04-18-10 22:04
cdrick  
kwl  
normal  
closed 3.10  
fixed  
none    
none trunk  
0006937: removeAll: aCollection doesn't do what we expect if aCollection==self
This known error has had a revival originating from a mail of Sophie in the beginnner list... and I ended up opening a bug report...

The problem is:

col := #(1 2 3) asOrderedCollection.
col removeAll: col.
^col returns an OrderedCollection(2) instead of an empty one as we would expect...
Explanation:

"If you start removing items from the collection, increasing the
index will cause you to skip elements." from Ron


Ron suggested:
"I agree it is an error, but it's one that you hit pretty early in your
Smalltalk career, I'm not sure anything really needs to change. I would
support just using a copy on the parameter and adding #removeAll: Is this
necessary? It wouldn't hurt."

col := #(1 2 3) asOrderedCollection.
col removeAll: col **copy**.
^col returns an OrderedCollection()
  
Goran:
"Because modifying the collection while iterating over the same is not
safe generally. The above idiom is "known" - but of course it sucks. :)

I advocated to add a #removeAll message in the protocol of Collection
and then implement it in subclasses using efficient variants a looooong
while back but it never got accepted. The climate today for such a
change is much more... "pragmatic" I think and hope. So we should simply
add it!"

Concerning using copy...
Klaus:
"Thumbs down, I do not want anybody copy a collection behind my back
(however small or large it is) just because remove* has nothing to do with
copy."

I was suggesting raising an error as, it's quite "strange" (circular) to have an expression like: aCol removeAll: aCol ...

But Klaus promised a fix so ;)

related to 0007177closed andreas removeAll fails for Dictionaries 
 OrderedCollection-removeAll-kwl-6937.st [^] (562 bytes) 02-19-08 18:51

Notes
(0011843)
kwl   
02-19-08 18:51   
OrderedCollection-removeAll-kwl-6937.st tested with the 441 cases in the CollectionTests category, all green.
(0011846)
nicolas cellier   
02-19-08 20:28   
"As posted in beginners list, self == aCollection does not handle case when aCollection is just a wrapper on self...
So the change won't removeAll problems...
in a 3.9 image try this:"

| collec1 collec2 |
collec1 := OrderedCollection with: 'a' with: 'b' with: 'c'.
collec2 := MappedCollection collection: collec1 map: (3 to: 1 by: -1).
collec1 removeAll: collec2.
collec1 inspect

"Agree, yet, the problem lies in super...
Note: MappedCollection was removed from 3.10, but in spirit there can be other wrapper collections (i used some myself)."
(0011847)
nicolas cellier   
02-19-08 21:09   
"Oops, i copy/pasted the wrong test.
The one that fails is more obvious:"

| collec1 collec2 |
collec1 := OrderedCollection with: 'a' with: 'b' with: 'c'.
collec2 := MappedCollection collection: collec1 map: (1 to: 3).
collec1 removeAll: collec2.
collec1
(0011848)
kwl   
02-19-08 21:41   
@nicolas
as mentioned in beginners@lists.squeakfoundation.org, since 3.10 MappedCollection no longer exists. Do you have a counter example which fails indeed in Squeak 3.10; can you break the attached fix in 3.10?
(0011850)
nicolas cellier   
02-19-08 22:43   
Proposed change is OK to me. I vote for inclusion in 3.11.
It does not fail per se. super fails...

Simply, i wanted to point that it won't solve all the tricky cases.
So these cases will have to be handled at super level.
And adding a self == aCollection at super level will still have flaws.

MappedCollection is not mainstream anymore, but other wrappers could become, see LazyLists and LazyCollections in Squeak source...
Collection wrapper is a classical pattern.
As long as they are used under your control, no problem.
(0011861)
kwl   
02-22-08 23:35   
There are perhaps other collections (besides OrderedCollection) which would benefit from such fix.
(0012222)
Keith_Hodges   
05-29-08 17:35   
"fix begin"
Installer mantis bug:6937 fix: 'OrderedCollection-removeAll-kwl-6937.st'.
"fix end"
(0012864)
kwl   
12-18-08 05:21   
sometimes I do no understand what "status" this Mantis thing wants to have changed, there seemingly are 3 of them :(

I hope that this post influences one of the stati towards the positive, the fix was attached long time ago.
(0013306)
nicolas cellier   
09-14-09 20:03   
I tried to generalize the case in 0007177
There, an alternate #removeAll: implementation exploits the #removeAll message.
#removeAll message can be found in
- Keith's Kernel-Extensions at http://www.squeaksource.com/311 [^]
- 0007177
- in recent squeak trunk http://source.squeak.org/trunk [^]
(0013335)
nicolas cellier   
10-04-09 20:37   
This was resolved by 0007177