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>>initializeWithName:
> 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>>initializeWithName:superclassName:
> traitComposition:classTraitComposition:category:instVarNames:
> classVarNames:poolDictionaryNames:classInstVarNames: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: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:

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].
and change the mocks to use #compiledCode instead of #bytes.  What do you
think?

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