The standard does *not* support - a removeAll: a - [was: Re: [BUG] Collection>>removeAll:]

Richard A. O'Keefe squeak-dev at lists.squeakfoundation.org
Wed Sep 11 03:04:06 UTC 2002


Jesse Welton <jwelton at pacific.mps.ohio-state.edu> wrote:
	Richard, I think you misunderstand Lex's point.  I believe it is that
	a reasonable programmer might rewrite a working call

	    a removeAll: b
	as
	    b do: [:each | a remove: each].
	
No, I didn't misunderstand it.  (This wasn't actually the example he
started with, which would _instantly_ have broken, but let it pass.)
But I had a point too, which you quoted:

	> 	> I thought it was generally agreed in this and related discussions
	> 	> that good programmers _ought_ to know that it is a bad idea to modify
	> 	> a container while it is running an iterator method.

	Now, if #removeAll: detects the special case a==b and handles it, it
	is possible that this case happens in practice, and the programmer's
	rewritten code will break silently.

The key point here is that if YOU rewrite working code, then YOU assume
FULL responsibility for ensuring that it still works.  This includes but
is not limited to
 - understanding your programming language
 - understanding the *specification* of the code you rewrote
 - understanding the specifications and limitations of the operations
   you use in the rewrite
 - all appropriate testing.

	On the other hand, is #removeAll:  detects a==b and throws an
	error, this case probably does not happen in practice, and the
	programmer's code will work.

Practice shmactice.  The case a==b most certainly SHOULD happen in
the programmer's testing.  The job of testing is not to try to simulate
normal "practice" but to try to ensure good quality code by putting it
to a serious test.

	Therefore, given that this is a reasonable
	transformation for the programmer to make, throwing an error is safer.

You've just explained why it isn't.  If the a==b case doesn't occur in
the programmer's code, it doesn't matter _what_ #removeAll: does then.
An error won't make it safer, because the ``error'' won't _happen_.

	I don't buy it, though.  I don't believe that it is, in general, a
	reasonable transformation to make.  It is longer and less intention
	revealing.

Those are good reasons why it's not a good transformation to *stop* at.
It still might make a good step along the way to doing something different,
e.g. if you didn't know about #removeAllFoundIn:.  Perhaps more importantly,
there is an excellent reason why it is an utterly implausible FIRST step.

Here's the reason.  If I want to transform a method call into something
else, I _don't_ slam down the dumbest code I can think of.  What I do is
pick up a copy of the method I expect to be called, and transform _that_.

So if the implementation read

removeAll: aCollection
    self assert: [aCollection ~~ self].
    aCollection do: [:each | self remove: each].
    ^aCollection

then _overwhelmingly_ the most probable starting point for a rewrite of

    a removeAll: b.

would be

    a assert: [b ~~ a]. "THIS CHECK COMES ALONG TOO!"
    b do: [:each | a remove: each].

	In any case, the moment you write the explicit loop, the special case
	becomes your responsibility, either way you think it should be
	handled.
	
And yes, that's the point I started with.  YOU write the loop => YOU check it.




More information about the Squeak-dev mailing list