Hi all!

(waving a white flag hysterically) :-)

Let me first tell you that this thread has been interesting and a
learning experience in many different ways, and not all about Squeak but
also rhetorics etc. ;-)

I now have a suggestion - why don't we move over into the "do it" phase?
Could Richard, which IMHO probably is the one who has this thread mostly
covered in his head, summarize a concrete proposal of a change?

If my memory serves me we have talked about:

1. Changing the base implementation of #removeAll: so that it doesn't do
fishy stuff when being called with itself as the argument. I don't want
to argue any finer points but IMHO Richard has at least proven beyond
doubt that the current situation is simply buggy and should be fixed
*somehow*.

We have heard a range of suggestions like fixing the comment (a bit like
chickening out if you ask me), signalling an exception, fixing the
implementation using copy (always or just when reciever == argument) and
various other optimization techniques so that it also gets faster.
Richard, give us your best shot.

2. Adding a #removeAll (no argument) in Collection with a nice base
implementation and then especially adding the concrete implementation in
OrderedCollection but also others perhaps.

3. Various other ideas/suggestions. (Perhaps I should comb through this
thread to find the other good bits.)


Anyway, to get us jumpstarted, here is a summary of what *I* think:

Personally :-) it still, probably unrationally, bothers me to *always*
do a copy to avoid any plausible aliasing problems. I would think that
perhaps trapping the "self == argument" situation is enough in Squeak
today (no slices etc) so I am instead leaning over to this
implementation:

Collection>>removeAll: aCollection
	"Remove each element of aCollection from the receiver. If successful
for 
	each, answer aCollection. Otherwise create an error notification.
	ArrayedCollections cannot respond to this message."

	|aBlock|
	self isEmpty ifTrue:[^aCollection].
	self == aCollection ifTrue: [^self removeAll].
	aBlock := [self errorNotFound].
	^aCollection do: [:each |
	    self remove: each ifAbsent: aBlock]]

- I check for empty as Stephan suggested. Richard may have other
opinions but it would at least guard against rather slow removing of
large collections from empty ones.
- I don't use assert but instead "==", I am not sure why assert: would
be better... Anticipation of being able to turn them off somehow in the
future? Well. IMHO this check should NOT be turned off ever.
- I use the optimization of Richard (block created once outside of loop,
using remove:ifAbsent: instead of remove:)
- I don't copy always, instead I call #removeAll and thus handles the
case of "x removeAll: x".
- I still return aCollection so the message still does what it should (I
think).

Then I added Collection>>removeAll (Gasp! How *dares* he change the
protocol of Collection! :-) because I can't see why we shouldn't have
such a method:

Collection>>removeAll
	"Remove all elements in the receiver by simply removing them
	one by one. This base implementation uses a copy of the receiver
	in order to avoid the problem of iterating over a changing collection.
	Subclasses should typically reimplement this method to be more
efficient."
 
	|aBlock|
	aBlock := [self errorNotFound].
	self copy do: [:each |
	    self remove: each ifAbsent: aBlock]].
	^self

The idea being that this implementation is rarely used since
OrderedCollection etc would use special implementations like for
example:

OrderedCollection>>removeAll
	"Remove all elements in the receiver."

     self setCollection: ( Array new: array size )

- I "reuse" array size simply guessing that it could easily grow back to
that size. Perhaps a smaller would be better as Tim suggested.


Ok, blast away.

regards, Göran

PS. I really think we should not be so *damn afraid* of
changing/improving the base Smalltalk classes. Sure, they have stood the
test of time, but everyone makes mistakes and everything can be
improved. IMHO. DS