The Trunk: Monticello-dtl.672.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

The Trunk: Monticello-dtl.672.mcz

commits-2
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
+ !


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: The Trunk: Monticello-dtl.672.mcz

Eliot Miranda-2
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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: The Trunk: Monticello-dtl.672.mcz

Eliot Miranda-2


On Wed, Aug 9, 2017 at 9:52 AM, Eliot Miranda <[hidden email]> 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, <[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].

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: The Trunk: Monticello-dtl.672.mcz

Eliot Miranda-2
Hi David,

    I saved the proposed changes to inbox

On Wed, Aug 9, 2017 at 9:58 AM, Eliot Miranda <[hidden email]> wrote:


On Wed, Aug 9, 2017 at 9:52 AM, Eliot Miranda <[hidden email]> 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, <[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].

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: The Trunk: Monticello-dtl.672.mcz

David T. Lewis
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
>



Loading...