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 + ! |
Hi David,
I like the intent of these changes but want to discuss the implementation. On Sun, Aug 6, 2017 at 9:25 AM, <[hidden email]> wrote: David T. Lewis uploaded a new version of Monticello to project The Trunk: 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 |
On Wed, Aug 9, 2017 at 9:52 AM, Eliot Miranda <[hidden email]> wrote:
Oops, I mean replace it with type := typeSymbol.
_,,,^..^,,,_ best, Eliot |
Hi David, I saved the proposed changes to inbox On Wed, Aug 9, 2017 at 9:58 AM, Eliot Miranda <[hidden email]> wrote:
_,,,^..^,,,_ best, Eliot |
In reply to this post by Eliot Miranda-2
Yes by all means, a better implementation would be welcome. I changed it
so as to be not-obviously-incorrect, but it is rather hackish both before and after the changes. By way of background, I stumbled across the issue while trying to bring my old V3 update stream up to date with the CompiledCode changes (I still have not gotten it to work, mainly due to my own limited understanding and recent lack of time). But I have /not/ seen any actual problems when working with my main Spur-64 trunk image. In fact I do not see these methods even being called when working with trunk. Nevertheless, the methods were obviously wrong as written, so I added some tests and updated the methods to be less horrible. Thanks, Dave > Hi David, > > I like the intent of these changes but want to discuss the > implementation. > > On Sun, Aug 6, 2017 at 9:25 AM, <[hidden email]> 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 > |
Free forum by Nabble | Edit this page |