Anonymous | Login | 01-15-2021 18:54 UTC |
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 |
![]() |
||||||||
|
![]() |
|
(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 |
Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
93 total queries executed. 54 unique queries executed. |