<div dir="ltr">Hi David,<div class="gmail_extra"><br></div><div class="gmail_extra"> I like the intent of these changes but want to discuss the implementation.</div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Aug 6, 2017 at 9:25 AM, <span dir="ltr"><<a href="mailto:commits@source.squeak.org" target="_blank">commits@source.squeak.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">David T. Lewis uploaded a new version of Monticello to project The Trunk:<br>
<a href="http://source.squeak.org/trunk/Monticello-dtl.672.mcz" rel="noreferrer" target="_blank">http://source.squeak.org/<wbr>trunk/Monticello-dtl.672.mcz</a><br>
<br>
==================== Summary ====================<br>
<br>
Name: Monticello-dtl.672<br>
Author: dtl<br>
Time: 6 August 2017, 12:25:45.326051 pm<br>
UUID: dfe33bfa-dc27-4c01-a1db-<wbr>234bd7e4433c<br>
Ancestors: Monticello-eem.671<br>
<br>
CompiledCode and its subclasses are a special case, and are identified as class type #compiledMethod rather than #bytes. An MCClassDefinition should identify this type correctly, otherwise changes to these classes may not be saved and reloaded correctly.<br>
<br>
See MCClassDefinitionTest.<br>
<br>
=============== Diff against Monticello-eem.671 ===============<br>
<br>
Item was changed:<br>
----- Method: MCClassDefinition>><wbr>initializeWithName:<wbr>superclassName:category:<wbr>instVarNames:classVarNames:<wbr>poolDictionaryNames:<wbr>classInstVarNames:type:<wbr>comment:commentStamp: (in category 'initializing') -----<br>
initializeWithName: nameString<br>
superclassName: superclassString<br>
category: categoryString<br>
instVarNames: ivarArray<br>
classVarNames: cvarArray<br>
poolDictionaryNames: poolArray<br>
classInstVarNames: civarArray<br>
type: typeSymbol<br>
comment: commentString<br>
commentStamp: stampStringOrNil<br>
name := nameString asSymbol.<br>
superclassName := superclassString ifNil: ['nil'] ifNotNil: [superclassString asSymbol].<br>
category := categoryString.<br>
+ (self isCompiledCode: name) ifTrue: [type := #compiledMethod] ifFalse: [type := typeSymbol].<br>
- name = #CompiledMethod ifTrue: [type := #compiledMethod] ifFalse: [type := typeSymbol].<br>
comment := commentString withSqueakLineEndings.<br>
commentStamp := stampStringOrNil ifNil: [self defaultCommentStamp].<br>
variables := OrderedCollection new.<br>
self addVariables: ivarArray ofType: MCInstanceVariableDefinition.<br>
self addVariables: cvarArray sorted ofType: MCClassVariableDefinition.<br>
self addVariables: poolArray sorted ofType: MCPoolImportDefinition.<br>
self addVariables: civarArray ofType: MCClassInstanceVariableDefinit<wbr>ion.!<br>
<br>
Item was changed:<br>
----- Method: MCClassDefinition>><wbr>initializeWithName:<wbr>superclassName:<wbr>traitComposition:<wbr>classTraitComposition:<wbr>category:instVarNames:<wbr>classVarNames:<wbr>poolDictionaryNames:<wbr>classInstVarNames:type:<wbr>comment:commentStamp: (in category 'initializing') -----<br>
initializeWithName: nameString<br>
superclassName: superclassString<br>
traitComposition: traitCompositionString<br>
classTraitComposition: classTraitCompositionString<br>
category: categoryString<br>
instVarNames: ivarArray<br>
classVarNames: cvarArray<br>
poolDictionaryNames: poolArray<br>
classInstVarNames: civarArray<br>
type: typeSymbol<br>
comment: commentString<br>
commentStamp: stampStringOrNil<br>
name := nameString asSymbol.<br>
superclassName := superclassString ifNil: ['nil'] ifNotNil: [superclassString asSymbol].<br>
traitComposition := traitCompositionString.<br>
classTraitComposition := classTraitCompositionString.<br>
category := categoryString.<br>
+ (self isCompiledCode: name) ifTrue: [type := #compiledMethod] ifFalse: [type := typeSymbol].<br>
- name = #CompiledMethod ifTrue: [type := #compiledMethod] ifFalse: [type := typeSymbol].<br>
comment := commentString withSqueakLineEndings.<br>
commentStamp := stampStringOrNil ifNil: [self defaultCommentStamp].<br>
variables := OrderedCollection new.<br>
self addVariables: ivarArray ofType: MCInstanceVariableDefinition.<br>
self addVariables: cvarArray sorted ofType: MCClassVariableDefinition.<br>
self addVariables: poolArray sorted ofType: MCPoolImportDefinition.<br>
self addVariables: civarArray ofType: MCClassInstanceVariableDefinit<wbr>ion.!<br>
<br>
Item was added:<br>
+ ----- Method: MCClassDefinition>><wbr>isCompiledCode: (in category 'initializing') -----<br>
+ isCompiledCode: className<br>
+ "Classes of type #compiledMethod are treated specially. CompiledCode<br>
+ and all of its subclasses, including CompiledMethod, should have this<br>
+ special type."<br>
+<br>
+ CompiledCode<br>
+ withAllSubclassesDo: [ :cls | (className = cls name)<br>
+ ifTrue: [ ^true ]].<br>
+ ^ false<br>
+ !<br>
</blockquote></div><br>I think that basing the type on the names of the subclasses of CompiledCode is error prone. For example, loading a package in which CompiledCode did /not/ have type #compiledMethod would result in an MCClassDefinition that reflected the current system rather than the package.</div><div class="gmail_extra"><br></div><div class="gmail_extra">I think the right thing is to base it on the the typeSymbol argument of MCClassDefinition>>name:superclassName:[traitComposition:classTraitComposition:]category:instVarNames:classVarNames:poolDictionaryNames:classInstVarNames:type:comment:commentStamp:. This should be #compiledMethod for CompiledCode and subclasses, derived from Behavior>>typeOfClass. But you're explicitly using #bytes for this in your mocks, as in:</div><div class="gmail_extra"><br></div><div class="gmail_extra">MCClassDefinitionTest>>testKindOfSubclassForCompiledBlock<div class="gmail_extra"><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>"CompiledCode and its subclasses are a special case."</div><div class="gmail_extra"><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>| classDef |</div><div class="gmail_extra"><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>"CompiledBlock should be like CompiledMethod, both now are subclassed from CompiledCode"</div><div class="gmail_extra"><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>classDef := self mockClass: #CompiledBlock super: #CompiledCode type: #bytes.</div><div class="gmail_extra"><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>self assert: #compiledMethod equals: classDef type.</div><div class="gmail_extra"><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>self assert: CompiledBlock typeOfClass equals: classDef type.</div><div><br></div><div>Surely the correct thing here is</div><div><br></div><div><div>testKindOfSubclassForCompiledBlock</div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>"CompiledCode and its subclasses are a special case."</div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>| classDef |</div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>"CompiledBlock should be like CompiledMethod, both now are subclassed from CompiledCode"</div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>classDef := self mockClass: #CompiledBlock super: #CompiledCode type: CompiledBlock typeOfClass.</div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>self assert: #compiledMethod equals: classDef type.</div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>self assert: CompiledBlock typeOfClass equals: classDef type.</div></div><div><br></div><div>no?</div><div><br></div><div>It looks to me like the statement</div><div> name = #CompiledMethod ifTrue: [type := #compiledMethod] ifFalse: [type := typeSymbol].<br></div><div>is an old hack for p[re-Spur which didn't bother to distinguish between CompiledMethod and subclasses and #bytes classes such as ByteArray. But at least since 1999 typeOfClass has answered #compiledMethod for CompiledMethod-like classes.</div><div><br></div><div>So I propose to delete the line</div><div> (self isCompiledCode: name) ifTrue: [type := #compiledMethod] ifFalse: [type := typeSymbol].<br></div><div>and change the mocks to use #compiledCode instead of #bytes. What do you think?</div><div><br></div><div class="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div>
</div></div>