Mantis Bugtracker
  

Viewing Issue Advanced Details Jump to Notes ] View Simple ] 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 Platform
Status closed   OS
Projection none   OS Version
ETA none Fixed in Version trunk Product Version 3.10
  Product Build
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...
Steps To Reproduce
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