[BUG]Collection>>removeAll:

Richard A. O'Keefe ok at cs.otago.ac.nz
Thu Aug 22 05:32:35 UTC 2002


"David Griswold" <David.Griswold at acm.org> wrote:
	You made a blanket assertion, that methods should not ever return
	garbage silently, and that it is their responsibility to make sure that
	such results are prevented.

Procedures, functions, methods or whatever should be so designed that they
do not return garbage silently.

There.  I did it again.

Remember, I am not talking about generalities.  I am talking about specific
methods in a specific library.

I continue to affirm that if a call to a method

    MEETS THE STATED PRECONDITIONS

so that the caller is innocent, and yet the method

    DOES NOT ACHIEVE ITS STATED POSTCONDITION

then the method has an obligation to yell about it and not silently
return garbage.

We aren't talking about methods that are *documented* not to work in
a certain special case.  We are talking about methods whose name and
comments lead the reader to believe that they _will_ work in that
case, but they don't.

We are not even talking about semantically odd cases.
There is nothing odd about X \ X or X U X when X is a mathematical set;
and if your operation is X := X \ Y, this is perfectly well defined when
X = Y.

I also affirm that it is a good thing if an expensive method (remember,
I am dealing in specifics here) checks such parts of its precondition as
are cheap (in context).

	I entirely agree that not having such aliasing problems would be
	fantastic; but all the languages you listed are non-imperative, and
	Smalltalk is imperative.

No, I have also mentioned AWK, which is entirely imperative.

	I love the fact that this kind of problem
	doesn't happen in non-imperative languages, but that doesn't make
	Smalltalk one, and it doesn't make it possible to make Smalltalk act
	that way in general, which is what your blanket statement requires.
	
No, I don't have a blanket statement.  I have a statement IN A PARTICULAR
CONTEXT, which is methods called with arguments valid according to the
precondition but not achieving their postcondition.

	All the things you want in terms of documentation, and detection of
	interface violations are good, but we disagree profoundly about where
	such checks belong.

They belong in the cheapest most maintainable place.

That *usually* means in the method itself, not in its callers.

	I think that they simply don't in general belong in
	the main runtime code path, they belong in separate assertions or
	pre/postconditions whose cost can be made zero in production.
	
But we are, or I am, explicitly *NOT* talking "in general".
I am talking about expensive (O(N)) operations and cheap (O(1)) checks.
I try not to be an ideological purist, and I don't see a great practical
difference between "cost is zero" and "cumulative cost is so small it is
extremely hard to detect."

That's one reason why I prefer the "patch #removeAll: and #addAll:" solution
to the "fix #do:" solution.

	We also disagree profoundly about whether (x removeAll: x) makes sense
	*as the library is written*.

As the library is *implemented*, no it does not make sense.
That's because the library is ***buggy***.

I don't think the question "makes sense as the library is written" is a
useful question.

The point is that the operation

    x.add_all_elements_of(y);

MAKES SENSE even when y is x, and the operation

    x.remove_all_elements_of(y);

MAKES SENSE even why y is x, and these operations CAN be implemented easily
in Smalltalk, and the method comments lead you to believe that these are
the operations you have been provided with.

	I think it just doesn't, and I explained
	why, so this input is not part of the expected domain of the method.

Your reason boils down to "if the implementor uses dumb code, it won't work;
always expect the implementor to use dumb code."  That isn't practical
advice.  It is the job of a library *designer* to provide a clean interface
with as few surprises as practically possible.  It is the job of a library
*implementor* to implement that interface; and where it turns out to be
impractical, the documentation should reflect the change.

I've *been* a library designer.  I've *been* a library implementor.
I know that it is possible to avoid this kind of bug.

	You disagree- 'nuf said.  However, I think that it is a common bug that
	almost everyone makes at some point, and so catching it would be a "good
	thing".   I think the reason that passing the receiver as an argument is
	not supported right now is that most often, that's not the most
	efficient way to do it, so it is not necesarily desirable to encourage
	emptying a collection this way.  It is almost always more efficient and
	just as convenient, to just create a new collection.   Extending the
	library to support using (x removeAll: x) to empty a collection in-place
	sounds potentially reasonable to me if its importance could be
	justified- but I do not believe the method as currently written is
	broken, and that is our real disagreement.
	
The implementation of the method does not agree with ANY of the documentation	
I could find for it.
	
If that's not a good enough approximation of "broken", what the heck WOULD be?



More information about the Squeak-dev mailing list