[BUG]Collection>>removeAll:

Göran Hultgren goran.hultgren at bluefish.se
Wed Aug 28 19:33:54 UTC 2002


Hi all!

Quoting "Andrew C. Greenberg" <werdna at mucow.com>:
> On Wednesday, August 28, 2002, at 06:12 AM, goran.hultgren at bluefish.se 
> wrote:
> 
> >> - it is clear that people do try to write (x removeAll: x), and that
> 
> >> it
> >> doesn't do what they expect.  This seems to happen frequently enough
> >> that it would be desirable to at least let people know when this
> >> happens, so they can write their code a different way.  The
> currently
> >> accepted valid ways to empty a collection are to either create a new
> >> collection (usually the fastest way) or say (x removeAll: x copy).
> >
> > A few remarks:
> > - The situation can occur without it being written "x removeAll: x"
> > intentionally by the developer. Rather "x removeAll:
> > (some-funky-expression-that-just-happens-to-evaluate-to-x)".
> 
> This is handled by the assertion proposal.

Yes, I know. I was just trying to emphasize the fact that the situation can
occur without having coded it explicitly. A developer typing "x removeAll: x"
would probably reflect on the fact that it could mean trouble. But as I said,
the situation could be "hidden" in the code.

> > - Creating a new collection does not retain identity.
> 
> Yes, this is the reason why it is valuable to have a method to remove 
> all elements in some cases.  It does not suggest that an implicit 
> iteration method should break Smalltalk's "don't change the iterating 
> collection" semantics rule for just a few special cases.

Excuse me for my ignorance in this area, but:

1. I (and a newbie I guess) wouldn't consider removeAll: to be an iteration
operation. That to me seems to be an implementation thing.
2. Where is this "semantics rule" you mention documented? I haven't read many
Smalltalk books...

> > - "x removeAll: x copy" is really IMHO much more silly than "x
> > removeAll". It demands that you are aware that "x removeAll: x"
> doesn't
> > work and also that you realize that just sending in a copy would do
> the
> > trick. Not at all obvious, but finding a method called "removeAll" is
> > very obvious.
> 
> Hardly.  It is an idiom of the language, and those familiar with most 
> collection iterations do this sort of thing routinely.

Ok, I have been coding Smalltalk for many years and not until this thread popped
up had I really reflected on the fact that "x removeAll: x copy" would be the
"obvious" way of emptying a collection. I do not do this routinely. And you can
not convince me that a newbie would "guess" this.

> >> - If we come up with a way to detect this case and generate an
> error,
> >> then the only remaining rationale for extending removeAll: to
> support
> >> the special case of the receiver as an argument would be if it was
> >> required by ANSI, or made the library or its specification simpler
> and
> >> cleaner, or fixed an actual bug.  I believe that none of these has 
> >> been
> >> demonstrated to be true.
> >
> > Why would that be "the only remaining rationale"? Honestly, I am not
> > joking?
> > A good reason IMHO is to improve the class. Squeak is not bound by 
> > ANSI.
> > Sure, being ANSI compliant (which it isn't) is a good thing, but NOT
> if
> > it stops us from improvement.
> 
> I think David is saying that he doesn't see any requirements for 
> improvement, once ad hoc "would-be-nice-creeping-featuritis" is removed
> from the notion of an "improvement."  There are a kazillion ways to 
> provide the functionality you require.

I must say that I am on Richard side in this. We are not talking "featuritis",
we are fixing buggy code. Noone can convince me that:

|oc|
o := OrderedCollection with: 1 with: 2.
o removeAll: o.
o

Should give the result it does. NO WAY. An assert (as I gather you favour) is
much better than the current situation, but I still can't really understand why
we just couldn't "repair" removeAll: so that it works as expected. And I can't
really see what "iteration semantics" has got to do with it...

> > If we see a "developer trap" (=a good possibility of making
> mistakes),
> > which I surely would call this, then we should try to fix it.
> 
> David's assertion proposal addresses this without changing the 
> semantics.

So the semantics you keep mentioning, what is that? I guess we are talking about
"Do not change the collection during an iteration over the same, it is not
defined." (or something like that)

And where am I iterating when I type: "x removeAll: x"? I have no idea what x
will do to accomplish the goal of removing all those elements I am sending in -
I could guess there might be an iteration in there somewhere but why should I
need to know that?

> > (It's quite exactly the same problem as using removeAllSuchThat: on
> an
> > ArrayedCollection.
> >
> > 'abc' removeAllSuchThat: [:c | c = $d ]
> 
> The problem here is not that it misbehaves for $a, but that it behaves 
> at all for $d.   Assertions would solve the problem here as well.  (It 
> is an awkward property of the Collection hierarchy as is that #remove 
> is defined for immutable Collections).

Sure. It was a pretty far fetched comparison, I was trying to show that methods
failing "silently" because of unexpected input is simply buggy IMHO.

> > I agree with almost everything you said here about asserts except for
> > the fact that I would not do an assert in this case and instead call
> > removeAll on self and thus make the code work. :-) What is your 
> > argument
> > for not doing that? (an honest question)
> 
> Because it changes semantics by performing an implicit iteration on a 
> collection that changes during the operation.  This is undefined in 

Hmmm. I can't see where the "implicit iteration" takes place. With my proposed
changes (check for identity calls #removeAll instead) no iteration would take
place - the collection would simply realize it should empty itself and just does
that.

And by the way, what do you think of removeAllSuchThat: ? It also changes the
underlying colllection during the operation. But it uses a copy to get away with
it... Perhaps we should then remove that method according to these arguments? Or
perhaps I misunderstood it.

> most Smalltalks (and as many have persuasively argued, this is as it 
> should be).  By encouraging these special case expectations, we step 
> into the land of the dangerous, handling only one of a host of possible
> cases.  The iterand, implicit or otherwise, should not be modified 
> during such a loop.

I understand what you are saying, but I don't agree that #removeAll: should be
considered to be an "iteration method".

> >> However, the large collection argument is a good one, I think.  The
> >> question then becomes, where does this method belong?  One could
> argue
> >> that code dealing with such large collections almost always knows a 
> >> lot
> >> more about the collection than just that it is a collection that
> >> supports remove:, so adding efficient removeAll methods to specific
> >> collection classes could be done without adding it to the Collection
> >> protocol itself.
> >
> > But why? It is such a basic message that I was actually perplexed to 
> > see
> > it wasn't in there already.
> > But instead I find rather exotic messages like #removeAllSuchThat:
> ...
> 
> I have used the latter with some frequency, in fact, and haven't really
> 
> had an application where i desired a #removeAll.  In practice, I never 
> gave a second thought but to use the #removeAll: self copy solution.

But surely you agree that "x removeAll" is much more precise and obvious than
coming up with "x removeAll: x copy"? I mean, just standing there in front of a
newbie Smalltalker and saying "No, there is no message for emptying a
collection. Instead you write 'x removeAll: x copy'. Isn't that obvious?" Nah.
Don't buy it.

> > And again, why shouldn't it be used? And to be so overly defensive of
> > adding (note, we are not changing an existing method, we are simply
> > *adding* one) a method seems very counter productive to me. It would
> > more or less totally hinder any evolution at all in the Collection
> > hierarchy...
> 
> Many of David's "us" are of the view that we are "fixing" something 
> that isn't broken.  Evolution of Collection is good.  Not all of "us" 
> see this as even neutral.

I didn't understand the last sentence.

regards, Göran

PS. I saw the commit emails, did you get the RePlugin up there alright? DS

Göran Hultgren, goran.hultgren at bluefish.se
GSM: +46 70 3933950, http://www.bluefish.se
\"Department of Redundancy department.\" -- ThinkGeek



More information about the Squeak-dev mailing list