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 havethis
special type."CompiledCodewithAllSubclassesDo: [ :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