Mantis Bugtracker
Main | My View | View Issues | Change Log | Docs
  

Viewing Issue Simple Details [ Jump to Notes ] [ View Advanced ] [ Issue History ] [ Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0006937 [Squeak] Collections minor always 02-19-08 18:12 04-18-10 22:04
Reporter cdrick View Status public  
Assigned To kwl
Priority normal Resolution fixed  
Status closed   Product Version 3.10
Summary 0006937: removeAll: aCollection doesn't do what we expect if aCollection==self
Description 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...
Additional Information 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 ;)
Attached Files  OrderedCollection-removeAll-kwl-6937.st [^] (562 bytes) 02-19-08 18:51

- Relationships
related to 0007177closed andreas removeAll fails for Dictionaries 

- Notes
(0011843 - 109 - 109 - 109 - 109 - 109 - 109)
kwl
02-19-08 18:51

 OrderedCollection-removeAll-kwl-6937.st tested with the 441 cases in the CollectionTests category, all green.
 
(0011846 - 556 - 642 - 642 - 642 - 642 - 642)
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 - 261 - 313 - 313 - 313 - 313 - 313)
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 - 214 - 220 - 278 - 278 - 278 - 278)
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 - 535 - 589 - 589 - 589 - 589 - 589)
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 - 98 - 98 - 98 - 98 - 98 - 98)
kwl
02-22-08 23:35

 There are perhaps other collections (besides OrderedCollection) which would benefit from such fix.
 
(0012222 - 97 - 129 - 129 - 129 - 129 - 129)
Keith_Hodges
05-29-08 17:35

 "fix begin"
Installer mantis bug:6937 fix: 'OrderedCollection-removeAll-kwl-6937.st'.
"fix end"
 
(0012864 - 227 - 249 - 249 - 249 - 249 - 249)
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 - 286 - 316 - 538 - 704 - 704 - 704)
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 - 26 - 26 - 26 - 109 - 109 - 109)
nicolas cellier
10-04-09 20:37

 This was resolved by 0007177
 

- Issue History
Date Modified Username Field Change
02-19-08 18:12 cdrick New Issue
02-19-08 18:51 kwl File Added: OrderedCollection-removeAll-kwl-6937.st
02-19-08 18:51 kwl Note Added: 0011843
02-19-08 20:28 nicolas cellier Note Added: 0011846
02-19-08 21:09 nicolas cellier Note Added: 0011847
02-19-08 21:41 kwl Note Added: 0011848
02-19-08 22:43 nicolas cellier Note Added: 0011850
02-22-08 23:35 kwl Note Added: 0011861
02-22-08 23:35 kwl Assigned To  => kwl
02-22-08 23:35 kwl Status new => assigned
05-29-08 17:35 Keith_Hodges Note Added: 0012222
12-17-08 04:35 Keith_Hodges Issue Monitored: Keith_Hodges
12-17-08 04:35 Keith_Hodges Status assigned => acknowledged
12-18-08 05:21 kwl Status acknowledged => resolved
12-18-08 05:21 kwl Resolution open => fixed
12-18-08 05:21 kwl Note Added: 0012864
01-10-09 02:00 Keith_Hodges Status resolved => pending
01-10-09 02:27 Keith_Hodges Status pending => testing
01-10-09 03:39 Keith_Hodges Status testing => resolved
01-10-09 03:39 Keith_Hodges Fixed in Version  => 3.11
01-10-09 03:41 Keith_Hodges Status resolved => testing
09-14-09 17:26 nicolas cellier Relationship added related to 0007177
09-14-09 20:03 nicolas cellier Note Added: 0013306
10-04-09 20:37 nicolas cellier Status testing => resolved
10-04-09 20:37 nicolas cellier Fixed in Version 3.11 => trunk
10-04-09 20:37 nicolas cellier Note Added: 0013335
04-18-10 22:04 andreas Status resolved => closed

Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
93 total queries executed.
54 unique queries executed.
Powered by Mantis Bugtracker