On Wed, Aug 9, 2017 at 9:52 AM, Eliot Miranda <eliot.miranda@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@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].

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