"Richard A. O'Keefe" <ok at cs.otago.ac.nz> wrote:
[SNIP]
> Expect these methods in a [FIX] coming soon.

Good!

> 	And another thing, the implementation in Collection:
>     
>     removeAllSuchThat: aBlock 
>         "Evaluate aBlock for each element and remove all that
>         elements from the receiver for that aBlock evaluates to
>         true.  Use a copy to enumerate collections whose order
>         changes when an element is removed (i.e. Sets)."
>     
>         self copy do: [:each |
>             (aBlock value: each) ifTrue: [self remove: each]]
>     
>     Should the base implementation really use a copy here? I tried:
>     
>         self removeAll: (self select: [:each | aBlock value: each])
>     
> Why not
> 	self removeAll: (self select: aBlock)

Duh. Of course. :-)

>     Also note that I could add a "return" here,
> 
> This method doesn't have a defined value.  What would the return return,
> if not self?

I was merely noting that perhaps it would be more logical if it had
similar semantics as removeAll:.
But sure, then we should change the comment stating so etc, not
important.

> Given that #select: basically makes a copy, it isn't clear that there's
> much scope for improvement here.

I benchmarked my version a bit and true - it became slower.
It still irritates me that a potentially quite large collection is fully
copied if we say remove 1 instance from it using this method.

>     [He considered overriding #removeAllSuchThat: for LinkedList,]

As an experiment hopefully showing that the base implementation is far
slower than a specialized implementation.
This was due to my annoyance that the BASE implementation uses the copy
since it uses a copy, excerpt from comment:

"...Use a copy to enumerate collections whose order changes when an
element is removed (i.e. Sets)."

But that sort of assumes that all other subclasses of Collection (that
this does NOT apply to, like LinkedList for example) will have to note
this and remember to make a more suitable implementation for themselves.
I would say that this base implementation sounds like it should belong
to Set (or a currently nonexistant abstract base class for such
collections).

>     But when I was going to compare it with the inherited version I suddenly
>     discovered that the inherited version doesn't even work! ;-) #copy
>     doesn't work for LinkedList.
> 
> The [FIX] mentioned above addresses this.  It's not clear that there IS a
> satisfactory solution, because the LinkedList designers tried to make the
> internal structure of the list (the Link objects) its elements.  This is in
> marked contrast to most other collections, where it is perfectly possible
> for the same object to be an element of two collections, and where
> collections can, in general, hold any kind of object.  Basically, LinkedList
> needs to be thrown away and redesigned from scratch.
>
> While testing the new LinkedList>>shallowCopy, I tried
>     ell includesAnyOf: ell copy
> only to be told that LinkedLists are not indexable.
> 
> If they are not indexable, why do they inherit from SequenceableCollection?

The Collection hierarchy sometimes doesn't feel "sound". For example all
that stuff about remove: not being allowed in subclasses, and then
suddenly they work in OrderedCollection etc. I mean, I am no freak when
it comes to these things, pragmatism is quite ok, but sometimes I wonder
if it couldn't be better even though still using single inheritance.

> It's easy enough to implement LinkedList>>at:, though not fast...

Of course. :-)

> OK, I've just sent the [FIX].  It was developed in Squeak 2.8 and has been
> checked out in Squeak 3.0 and Squeak 3.2.  removeAll-raok.2.cs supersedes
> removeALl-raok.1.cs
> 
> There's another issue.
> 
> More-or-less parallel to the #remove* family there's a #copyWithout* family.
> But they're not QUITE parallel...

Oh, no.... :-)

Btw, I agree with you that we at least should fix the method comment in
removeAll:. I mean, that never hurts.


regards, Göran