<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 9, 2017 at 9:52 AM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</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"><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"><div><div class="gmail-h5"><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/trunk<wbr>/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-234bd7<wbr>e4433c<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>>initializeW<wbr>ithName:superclassName:<wbr>category:instVarNames:<wbr>classVarNames:poolDictionaryNa<wbr>mes:classInstVarNames:type:com<wbr>ment: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>>initializeW<wbr>ithName:superclassName:traitCo<wbr>mposition:classTraitCompositio<wbr>n:category:instVarNames:classV<wbr>arNames:poolDictionaryNames:cl<wbr>assInstVarNames:type:comment:<wbr>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>>isCompiledC<wbr>ode: (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></div></div>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:<wbr>superclassName:[<wbr>traitComposition:<wbr>classTraitComposition:]<wbr>category:instVarNames:<wbr>classVarNames:<wbr>poolDictionaryNames:<wbr>classInstVarNames:type:<wbr>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>><wbr>testKindOfSubclassForCompiledB<wbr>lock<div class="gmail_extra"><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>"CompiledCode and its subclasses are a special case."</div><div class="gmail_extra"><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>| classDef |</div><div class="gmail_extra"><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>"CompiledBlock should be like CompiledMethod, both now are subclassed from CompiledCode"</div><div class="gmail_extra"><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>classDef := self mockClass: #CompiledBlock super: #CompiledCode type: #bytes.</div><div class="gmail_extra"><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>self assert: #compiledMethod equals: classDef type.</div><div class="gmail_extra"><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </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>testKindOfSubclassForCompiledB<wbr>lock</div><div><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>"CompiledCode and its subclasses are a special case."</div><div><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>| classDef |</div><div><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>"CompiledBlock should be like CompiledMethod, both now are subclassed from CompiledCode"</div><div><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>classDef := self mockClass: #CompiledBlock super: #CompiledCode type: CompiledBlock typeOfClass.</div><div><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>self assert: #compiledMethod equals: classDef type.</div><div><span class="gmail-m_-217496925163772552gmail-Apple-tab-span" style="white-space:pre-wrap"> </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><span class="gmail-"><div> name = #CompiledMethod ifTrue: [type := #compiledMethod] ifFalse: [type := typeSymbol].<br></div></span><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><span class="gmail-"><div> (self isCompiledCode: name) ifTrue: [type := #compiledMethod] ifFalse: [type := typeSymbol].<br></div></span></div></div></blockquote><div><br></div><div>Oops, I mean replace it with</div><div> </div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>type := typeSymbol.<br></div><div><br></div><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"><div dir="ltr"><div class="gmail_extra"><span class="gmail-"><div></div></span><div>and change the mocks to use #compiledCode instead of #bytes. What do you think?</div><div><br></div><div class="gmail-m_-217496925163772552gmail_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>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><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>