Hi David,

    I saved the proposed changes to inbox

On Wed, Aug 9, 2017 at 9:58 AM, Eliot Miranda <eliot.miranda at gmail.com>
wrote:

>
>
> On Wed, Aug 9, 2017 at 9:52 AM, Eliot Miranda <eliot.miranda at gmail.com>
> wrote:
>
>> Hi David,
>>
>>     I like the intent of these changes but want to discuss the
>> implementation.
>>
>> On Sun, Aug 6, 2017 at 9:25 AM, <commits at source.squeak.org> wrote:
>>
>>> David T. Lewis uploaded a new version of Monticello to project The Trunk:
>>> http://source.squeak.org/trunk/Monticello-dtl.672.mcz
>>>
>>> ==================== Summary ====================
>>>
>>> Name: Monticello-dtl.672
>>> Author: dtl
>>> Time: 6 August 2017, 12:25:45.326051 pm
>>> UUID: dfe33bfa-dc27-4c01-a1db-234bd7e4433c
>>> Ancestors: Monticello-eem.671
>>>
>>> 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.
>>>
>>> See MCClassDefinitionTest.
>>>
>>> =============== Diff against Monticello-eem.671 ===============
>>>
>>> Item was changed:
>>>   ----- Method: MCClassDefinition>>initializeW
>>> ithName:superclassName:category:instVarNames:classVarNames:
>>> poolDictionaryNames:classInstVarNames:type:comment:commentStamp: (in
>>> category 'initializing') -----
>>>   initializeWithName: nameString
>>>   superclassName: superclassString
>>>   category: categoryString
>>>   instVarNames: ivarArray
>>>   classVarNames: cvarArray
>>>   poolDictionaryNames: poolArray
>>>   classInstVarNames: civarArray
>>>   type: typeSymbol
>>>   comment: commentString
>>>   commentStamp: stampStringOrNil
>>>         name := nameString asSymbol.
>>>         superclassName := superclassString ifNil: ['nil'] ifNotNil:
>>> [superclassString asSymbol].
>>>         category := categoryString.
>>> +       (self isCompiledCode: name) ifTrue: [type := #compiledMethod]
>>> ifFalse: [type := typeSymbol].
>>> -       name = #CompiledMethod ifTrue: [type := #compiledMethod]
>>> ifFalse: [type := typeSymbol].
>>>         comment := commentString withSqueakLineEndings.
>>>         commentStamp := stampStringOrNil ifNil: [self
>>> defaultCommentStamp].
>>>         variables := OrderedCollection  new.
>>>         self addVariables: ivarArray ofType:
>>> MCInstanceVariableDefinition.
>>>         self addVariables: cvarArray sorted ofType:
>>> MCClassVariableDefinition.
>>>         self addVariables: poolArray sorted ofType:
>>> MCPoolImportDefinition.
>>>         self addVariables: civarArray ofType:
>>> MCClassInstanceVariableDefinition.!
>>>
>>> Item was changed:
>>>   ----- Method: MCClassDefinition>>initializeW
>>> ithName:superclassName:traitComposition:classTraitCompositio
>>> n:category:instVarNames:classVarNames:poolDictionaryNames:cl
>>> assInstVarNames:type:comment:commentStamp: (in category 'initializing')
>>> -----
>>>   initializeWithName: nameString
>>>   superclassName: superclassString
>>>   traitComposition: traitCompositionString
>>>   classTraitComposition: classTraitCompositionString
>>>   category: categoryString
>>>   instVarNames: ivarArray
>>>   classVarNames: cvarArray
>>>   poolDictionaryNames: poolArray
>>>   classInstVarNames: civarArray
>>>   type: typeSymbol
>>>   comment: commentString
>>>   commentStamp: stampStringOrNil
>>>         name := nameString asSymbol.
>>>         superclassName := superclassString ifNil: ['nil'] ifNotNil:
>>> [superclassString asSymbol].
>>>         traitComposition := traitCompositionString.
>>>         classTraitComposition := classTraitCompositionString.
>>>         category := categoryString.
>>> +       (self isCompiledCode: name) ifTrue: [type := #compiledMethod]
>>> ifFalse: [type := typeSymbol].
>>> -       name = #CompiledMethod ifTrue: [type := #compiledMethod]
>>> ifFalse: [type := typeSymbol].
>>>         comment := commentString withSqueakLineEndings.
>>>         commentStamp := stampStringOrNil ifNil: [self
>>> defaultCommentStamp].
>>>         variables := OrderedCollection  new.
>>>         self addVariables: ivarArray ofType:
>>> MCInstanceVariableDefinition.
>>>         self addVariables: cvarArray sorted ofType:
>>> MCClassVariableDefinition.
>>>         self addVariables: poolArray sorted ofType:
>>> MCPoolImportDefinition.
>>>         self addVariables: civarArray ofType:
>>> MCClassInstanceVariableDefinition.!
>>>
>>> Item was added:
>>> + ----- Method: MCClassDefinition>>isCompiledCode: (in category
>>> 'initializing') -----
>>> + isCompiledCode: className
>>> +       "Classes of type #compiledMethod are treated specially.
>>> CompiledCode
>>> +       and all of its subclasses, including CompiledMethod, should have
>>> this
>>> +       special type."
>>> +
>>> +       CompiledCode
>>> +               withAllSubclassesDo: [ :cls | (className = cls name)
>>> +                       ifTrue: [ ^true ]].
>>> +       ^ false
>>> + !
>>>
>>
>> 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.
>>
>> I think the right thing is to base it on the the typeSymbol argument of
>> MCClassDefinition>>name:superclassName:[traitComposition:cla
>> ssTraitComposition:]category:instVarNames:classVarNames:poo
>> lDictionaryNames: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:
>>
>> MCClassDefinitionTest>>testKindOfSubclassForCompiledBlock
>> "CompiledCode and its subclasses are a special case."
>> | classDef |
>> "CompiledBlock should be like CompiledMethod, both now are subclassed
>> from CompiledCode"
>> classDef := self mockClass: #CompiledBlock super: #CompiledCode type:
>> #bytes.
>> self assert: #compiledMethod equals: classDef type.
>> self assert: CompiledBlock typeOfClass equals: classDef type.
>>
>> Surely the correct thing here is
>>
>> testKindOfSubclassForCompiledBlock
>> "CompiledCode and its subclasses are a special case."
>> | classDef |
>> "CompiledBlock should be like CompiledMethod, both now are subclassed
>> from CompiledCode"
>> classDef := self mockClass: #CompiledBlock super: #CompiledCode
>> type: CompiledBlock typeOfClass.
>> self assert: #compiledMethod equals: classDef type.
>> self assert: CompiledBlock typeOfClass equals: classDef type.
>>
>> no?
>>
>> It looks to me like the statement
>>        name = #CompiledMethod ifTrue: [type := #compiledMethod] ifFalse:
>> [type := typeSymbol].
>> 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.
>>
>> So I propose to delete the line
>>        (self isCompiledCode: name) ifTrue: [type := #compiledMethod]
>> ifFalse: [type := typeSymbol].
>>
>
> Oops, I mean replace it with
>
> type := typeSymbol.
>
> and change the mocks to use #compiledCode instead of #bytes.  What do you
>> think?
>>
>> _,,,^..^,,,_
>> best, Eliot
>>
>
>
>
> --
> _,,,^..^,,,_
> best, Eliot
>



-- 
_,,,^..^,,,_
best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20170809/d7244c01/attachment.html>