Mantis - Squeak
Viewing Issue Advanced Details
1711 Collections minor always 08-18-05 19:04 12-17-08 03:57
KenCausey  
 
normal  
feedback  
open  
none    
none  
0001711: [FIX] Re: Refactoring Associations
"Andrew P. Black" <black@cse.ogi.edu>:
Andreas Raab" <andreas.raab@gmx.de> writes:

>To understand the effect have a look at the implementations of
>WeakKeyAssociation>>key and WeakKeyAssociation>>key: - this should make it
>clear that WeakKeyAssociations store their key in form of a weak array so
>that it can be garbage collected. Thus there isn't anything broken, except
>from the interpretation of the key iVar.

I figured out what was going wrong - one last WeakKeyAssociation
object of the old kind (with the array) was being created by the act
of patching in the new kind of WeakKeyAssociation. (Changing the
class hierarchy created an obsolete subclass, which had to be weakly
remembered...)

However, by the time that this errant object was created, the method
dictionary of its class had been zapped
(Behavior>>canZapMethodDictionary always returns true; perhaps it
should check to see if there are still any instances or the potential
to create any). Thus, this object didn't behave correctly, because
its key method was the one inherited from LookUpKey.

By doing a double renaming I at last have a change set for this
refactoring that can be loaded, at least, it loads fine into a clean
3.7beta #5969.

This changeset passes the tests that I posted a couple of weeks ago."
"
related to 0001722feedback  [ENH] Associations Please review and read the tests 
 AssociationRefact.Hand.6.cs.gz [^] (2,961 bytes) 08-18-05 19:05
 AssociationRefact.cs.gz [^] (3,113 bytes) 08-18-05 19:08
 AssociationRefactTests.cs.gz [^] (1,802 bytes) 08-18-05 19:08
 AsssociationRefactCleanUp.cs.gz [^] (957 bytes) 08-18-05 19:08

Notes
(0002417)
KenCausey   
08-18-05 19:05   
karl.ramberg@chello.se:

"Hi, Andrew. I'm not sure what this changeset does. Could you check it
against the 3.9alpha if there are any conflicts, and give a short
description in the preamble of what the change set does ?"
(0002418)
KenCausey   
08-18-05 19:07   
tomkoenig@mindspring.com:

"Tested with 3.9a#6506 including execution of the sunit tests Andrew
provided in a separated email.
Reviewed all code for quality and conflicts. The refactoring is not
crucial but it definitely makes these core classes cleaner. The work on
cleaning up the code, creating a usfule test suite for this important
set of classes and figuring out how to install the change definitely
merits inclusion in 3.9a. (There is no particular need to add it to
3.8b, although I did some of my orignal testing there.)
Background from preamble:
" tlk:Can you tell me something about the motivation for the original
Association Refactoring cs ?
ab: Essentially, the problem was that several methods (like printOn: !)
were missing from some subclasses of Association, like
ReadOnlyVariableBinding. Also, WeakKeyAssociations were a really ugly
hack. The solution seemed to be to refactor the class hierarchy. This
was not hard, but coming up with a changeset that would file in without
breaking the compiler half way through was a challenge!
This changeset was produced by hand. The order in which the various
classes are defined or redefined is critical."

AssociationRefact is the prereq for the other two cs's. It is Andrew's
original cs with a change to the preamble to explain the motivation.
AssociationRefactTest is tests the first cs and is a prereq for the
final cs. Andrew sent me this cs, all I did was rename it, add a
preamble and add a class comment for the test class. (I know, anal).
AssociationRefactCleanUp is my work. It includes class comments for the
association classes and a postscript to remove a now redundant test
class. You could accept the first cs, or the first two cs and
everything would be okay. But I think all three together make the
better package."

(attaching AssociationRefact.cs.gz, AssociationRefactTests.cs.gz, AsssociationRefactCleanUp.cs.gz)
(0002419)
KenCausey   
08-18-05 19:08   
ducasse@iam.unibe.ch:

"Pay attention this is a test to see if we can get experts looking at
changes.
So can someone with knowledge on association and compiler have a look at
this refactoring. I know that andrew spent a lot of time on it and I
would like to avoid to see it lost."
(0002420)
KenCausey   
08-18-05 19:10   
I was able to load the original changeset AssociationRefact.Hand.6.cs.gz into 3.8-6665-basic without errors but I did not check it further. Whenever I attempted to load any of the other 3 changesets I got a debugger.
(0012855)
Keith_Hodges   
12-17-08 03:37   
3.11 Feedback - Need to review the code relative to 3.10.2bc. We have atomic loading for change sets. Name the change set .mcs so that monticello will load it, and load with Installer (with atomic loading enabled).