On Tuesday, July 13, 2004, at 03:35 PM, Chris Muller wrote:

> There was a lot of intense discussion following my original post
> (http://lists.squeakfoundation.org/pipermail/squeak-dev/2004-June/ 
> 078915.html)
> that included the fix and SUnit test.  Since 3.7gamma is approaching  
> fast, I
> thought it would be appropriate to try to rejuvenate a review of this  
> simple
> bug fix.

The big discussion (which was mostly about a tangential issue) probably  
distracted/scared most of the potential reviewers away. :)

I just browsed through the discussion again, and it seems like the only  
possible remaining issue was that Lex suggested that it might be better  
to move in the direction of allowing nil keys in dictionaries, since  
there's not really any reason to not allow them.  (Except for the  
obvious practical reason that Dictionary is subclassed from Set and a  
lot of inherited Set behavior assumes that nils are not allowed.)

But basically I agree with you that the behavior needs to be made  
consistent one way or the other... either nil keys are allowed or they  
are not, and currently they are (mostly) not allowed.  It would  
probably be a lot more work to support nil keys, so your (small) fix  
seems reasonable.

However, this is exactly the sort of change (a signficant change to  
base collection classes, even though it is a "fix") which should happen  
during the alpha cycle, not at the last minute right before gamma.  So  
I'd recommed approving it for 3.8alpha. (but I'll wait a bit and see if  
there are any comments)

- Doug

p.s. By the way, your test confused me a bit at first while I was  
trying to figure out how it worked... I saw that you were putting nil  
in the *value* of the association, but you were testing afterward if  
the *key* was nil.

	| d foundNil |
	foundNil _ false.
	d _ WeakIdentityKeyDictionary new
		at: 'hello' copy put: nil;
		yourself.
	Smalltalk garbageCollectMost.
	d keysAndValuesDo: [ : k : v | k ifNil: [ foundNil ] ].
	self deny: foundNil

Eventually I figured out that the value in the association didn't  
matter.  But changing the fourth line here to "at: 'hello' copy put:  
'value' copy;" might make the test a bit more clear.  (And the test  
still seems to work properly.  Actually, if you then run the test  
before adding your fix, the variable #v in the block still equals  
'value', while #k is nil.  Not sure how that happens.  Only the key  
gets garbage collected?)