The Inbox: Kernel-cmm.1190.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

The Inbox: Kernel-cmm.1190.mcz

commits-2
Chris Muller uploaded a new version of Kernel to project The Inbox:
http://source.squeak.org/inbox/Kernel-cmm.1190.mcz

==================== Summary ====================

Name: Kernel-cmm.1190
Author: cmm
Time: 29 August 2018, 9:20:01.027647 pm
UUID: b5a78e5d-c7fe-4e5d-9673-ee6208534ae1
Ancestors: Kernel-eem.1189

- Remove unnecessary string literals from ClassBuilder>>#reservedNames.
- Fix and simplify ClassBuilder>>#validateInstvars:from:forSuper:.
        ClassBuilder>>#validateClassvars:from:forSuper: may benefit from a similar change.

=============== Diff against Kernel-eem.1189 ===============

Item was changed:
  ----- Method: ClassBuilder>>reservedNames (in category 'private') -----
  reservedNames
  "Return a list of names that must not be used for variables"
+ ^#(self super thisContext #true #false #nil)!
- ^#('self' 'super' 'thisContext' 'true' 'false' 'nil'
- self super thisContext #true #false #nil).!

Item was changed:
  ----- Method: ClassBuilder>>validateInstvars:from:forSuper: (in category 'validation') -----
+ validateInstvars: instVarArray from: oldClass forSuper: newSuper
- validateInstvars: instVarArray from: oldClass forSuper: newSuper
  "Check if any of the instVars of oldClass conflict with the new superclass"
+ | usedNames |
- | instVars usedNames temp |
- instVarArray isEmpty ifTrue:[^true]. "Okay"
- newSuper allowsSubInstVars ifFalse: [
- self error: newSuper printString, ' does not allow subclass inst vars. See allowsSubInstVars.'. ^ false].
-
- "Validate the inst var names"
  usedNames := instVarArray asSet.
+ oldClass ifNotNil: [ oldClass withAllSubclassesDo: [ : each | each == oldClass ifFalse: [usedNames addAll: each instVarNames] ] ].
+ "Anything to possibly conflict?"
+ usedNames isEmpty ifTrue: [ ^ true ].
+ newSuper allowsSubInstVars ifFalse:
+ [ self error: newSuper printString , ' does not allow subclass inst vars. See allowsSubInstVars.'.
+ ^ false ].
+ instVarArray asSet size = instVarArray size ifFalse:
+ [ self error: instVarArray asBag sortedCounts first value, ' is multply defined.'.
+ ^ false ].
+ (usedNames intersection: self reservedNames) ifNotEmpty:
+ [ : reservedWords | self error: reservedWords anyOne , ' is a reserved name'.
+ ^ false ].
+ newSuper ifNotNil:
+ [ | offendingVars |
+ "If any variable names in subclasses conflict names in the new superclass, report the offending class and instVar name."
+ newSuper withAllSuperclasses
+ detect: [ : each | (offendingVars := each instVarNames intersection: usedNames) notEmpty ]
+ ifFound:
+ [ : offendingSuperclass |
+ DuplicateVariableError new
+ superclass: offendingSuperclass ;
+ variable: offendingVars anyOne ;
+ signal: offendingVars anyOne , ' is already defined in ' , offendingSuperclass name ]
+ ifNone: [ "no name conflicts" ] ].
+ ^ true!
- usedNames size = instVarArray size
- ifFalse:[ instVarArray do:[:var|
- usedNames remove: var ifAbsent:[temp := var]].
- self error: temp,' is multiply defined'. ^false].
- (usedNames includesAnyOf: self reservedNames)
- ifTrue:[ self reservedNames do:[:var|
- (usedNames includes: var) ifTrue:[temp := var]].
- self error: temp,' is a reserved name'. ^false].
-
- newSuper == nil ifFalse:[
- usedNames := newSuper allInstVarNames asSet.
- instVarArray do:[:iv|
- (usedNames includes: iv) ifTrue:[
- newSuper withAllSuperclassesDo:[:cl|
- (cl instVarNames includes: iv) ifTrue:[temp := cl]].
- (DuplicateVariableError new)
- superclass: temp;
- variable: iv;
- signal: iv,' is already defined in ', temp name]]].
- oldClass == nil ifFalse:[
- usedNames := Set new: 20.
- oldClass allSubclassesDo:[:cl| usedNames addAll: cl instVarNames].
- instVars := instVarArray.
- newSuper == nil ifFalse:[instVars := instVars, newSuper allInstVarNames].
- instVars do:[:iv|
- (usedNames includes: iv) ifTrue:[
- (DuplicateVariableError new)
- superclass: oldClass;
- variable: iv;
- signal: iv,' is already defined in a subclass of ', temp name]]].
- ^true!


Reply | Threaded
Open this post in threaded view
|

Why is testMoveVarFromSubToSuperclass doing that? (was: The Inbox: Kernel-cmm.1190.mcz)

Chris Muller-3
Kernel-cmm.1190.mcz is my proposed fix for the bug identified by the
new #testDuplicateInstanceVariableError, for your review.

But while working on that I was shocked to notice what
testMoveVarFromSubToSuperclass is doing and testing for.  Resuming the
DuplicateVariableException so it could, truly, make interrogations
about an object that shouldn't exist (right?).  Very strange!

Best,
  Chris

On Wed, Aug 29, 2018 at 9:20 PM <[hidden email]> wrote:

>
> Chris Muller uploaded a new version of Kernel to project The Inbox:
> http://source.squeak.org/inbox/Kernel-cmm.1190.mcz
>
> ==================== Summary ====================
>
> Name: Kernel-cmm.1190
> Author: cmm
> Time: 29 August 2018, 9:20:01.027647 pm
> UUID: b5a78e5d-c7fe-4e5d-9673-ee6208534ae1
> Ancestors: Kernel-eem.1189
>
> - Remove unnecessary string literals from ClassBuilder>>#reservedNames.
> - Fix and simplify ClassBuilder>>#validateInstvars:from:forSuper:.
>         ClassBuilder>>#validateClassvars:from:forSuper: may benefit from a similar change.
>
> =============== Diff against Kernel-eem.1189 ===============
>
> Item was changed:
>   ----- Method: ClassBuilder>>reservedNames (in category 'private') -----
>   reservedNames
>         "Return a list of names that must not be used for variables"
> +       ^#(self super thisContext #true #false #nil)!
> -       ^#('self' 'super' 'thisContext' 'true' 'false' 'nil'
> -               self super thisContext #true #false #nil).!
>
> Item was changed:
>   ----- Method: ClassBuilder>>validateInstvars:from:forSuper: (in category 'validation') -----
> + validateInstvars: instVarArray from: oldClass forSuper: newSuper
> - validateInstvars: instVarArray from: oldClass forSuper: newSuper
>         "Check if any of the instVars of oldClass conflict with the new superclass"
> +       | usedNames |
> -       | instVars usedNames temp |
> -       instVarArray isEmpty ifTrue:[^true]. "Okay"
> -       newSuper allowsSubInstVars ifFalse: [
> -               self error: newSuper printString, ' does not allow subclass inst vars. See allowsSubInstVars.'. ^ false].
> -
> -       "Validate the inst var names"
>         usedNames := instVarArray asSet.
> +       oldClass ifNotNil: [ oldClass withAllSubclassesDo: [ : each | each == oldClass ifFalse: [usedNames addAll: each instVarNames] ] ].
> +       "Anything to possibly conflict?"
> +       usedNames isEmpty ifTrue: [ ^ true ].
> +       newSuper allowsSubInstVars ifFalse:
> +               [ self error: newSuper printString , ' does not allow subclass inst vars. See allowsSubInstVars.'.
> +               ^ false ].
> +       instVarArray asSet size = instVarArray size ifFalse:
> +               [ self error: instVarArray asBag sortedCounts first value, ' is multply defined.'.
> +               ^ false ].
> +       (usedNames intersection: self reservedNames) ifNotEmpty:
> +               [ : reservedWords | self error: reservedWords anyOne , ' is a reserved name'.
> +               ^ false ].
> +       newSuper ifNotNil:
> +               [ | offendingVars |
> +               "If any variable names in subclasses conflict names in the new superclass, report the offending class and instVar name."
> +               newSuper withAllSuperclasses
> +                       detect: [ : each | (offendingVars := each instVarNames intersection: usedNames) notEmpty ]
> +                       ifFound:
> +                               [ : offendingSuperclass |
> +                               DuplicateVariableError new
> +                                        superclass: offendingSuperclass ;
> +                                        variable: offendingVars anyOne ;
> +                                        signal: offendingVars anyOne , ' is already defined in ' , offendingSuperclass name ]
> +                       ifNone: [ "no name conflicts" ] ].
> +       ^ true!
> -       usedNames size = instVarArray size
> -               ifFalse:[       instVarArray do:[:var|
> -                                       usedNames remove: var ifAbsent:[temp := var]].
> -                               self error: temp,' is multiply defined'. ^false].
> -       (usedNames includesAnyOf: self reservedNames)
> -               ifTrue:[        self reservedNames do:[:var|
> -                                       (usedNames includes: var) ifTrue:[temp := var]].
> -                               self error: temp,' is a reserved name'. ^false].
> -
> -       newSuper == nil ifFalse:[
> -               usedNames := newSuper allInstVarNames asSet.
> -               instVarArray do:[:iv|
> -                       (usedNames includes: iv) ifTrue:[
> -                               newSuper withAllSuperclassesDo:[:cl|
> -                                       (cl instVarNames includes: iv) ifTrue:[temp := cl]].
> -                               (DuplicateVariableError new)
> -                                       superclass: temp;
> -                                       variable: iv;
> -                                       signal: iv,' is already defined in ', temp name]]].
> -       oldClass == nil ifFalse:[
> -               usedNames := Set new: 20.
> -               oldClass allSubclassesDo:[:cl| usedNames addAll: cl instVarNames].
> -               instVars := instVarArray.
> -               newSuper == nil ifFalse:[instVars := instVars, newSuper allInstVarNames].
> -               instVars do:[:iv|
> -                       (usedNames includes: iv) ifTrue:[
> -                               (DuplicateVariableError new)
> -                                       superclass: oldClass;
> -                                       variable: iv;
> -                                       signal: iv,' is already defined in a subclass of ', temp name]]].
> -       ^true!
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Why is testMoveVarFromSubToSuperclass doing that? (was: The Inbox: Kernel-cmm.1190.mcz)

Eliot Miranda-2
Hi Chris,

On Wed, Aug 29, 2018 at 7:33 PM Chris Muller <[hidden email]> wrote:
Kernel-cmm.1190.mcz is my proposed fix for the bug identified by the
new #testDuplicateInstanceVariableError, for your review.

But while working on that I was shocked to notice what
testMoveVarFromSubToSuperclass is doing and testing for.  Resuming the
DuplicateVariableException so it could, truly, make interrogations
about an object that shouldn't exist (right?).  Very strange!

Imagine this:

Object subclass: #Fu
           instanceVariableNames: ''.
 
Fu subclass: #Bar
           instanceVariableNames: 'fubar'.

and we want to refactor to this:

Object subclass: #Fu
           instanceVariableNames: 'fubar'.
 
Fu subclass: #Bar
           instanceVariableNames: ''.

either we provide a class builder that can morph more than one class at a time or we have a problem.

If we refactor via this route:

1. { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar instanceVariableNames: 'fubar' }
=> { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar instanceVariableNames: '' }
=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar instanceVariableNames: '' }
then fubar becomes undeclared (which is a single l-value in Undeclared), and hence all instances of Bar but the first one have their inst var trashed

If we refactor via this route:
2.
1. { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar instanceVariableNames: 'fubar' }
=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar instanceVariableNames: 'fubar' }
=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar instanceVariableNames: '' }

we have a DuplicateVariableException to handle, but state could conceivably be preserved across the refactoring.

Does that explain the problem?

Best,
  Chris

On Wed, Aug 29, 2018 at 9:20 PM <[hidden email]> wrote:
>
> Chris Muller uploaded a new version of Kernel to project The Inbox:
> http://source.squeak.org/inbox/Kernel-cmm.1190.mcz
>
> ==================== Summary ====================
>
> Name: Kernel-cmm.1190
> Author: cmm
> Time: 29 August 2018, 9:20:01.027647 pm
> UUID: b5a78e5d-c7fe-4e5d-9673-ee6208534ae1
> Ancestors: Kernel-eem.1189
>
> - Remove unnecessary string literals from ClassBuilder>>#reservedNames.
> - Fix and simplify ClassBuilder>>#validateInstvars:from:forSuper:.
>         ClassBuilder>>#validateClassvars:from:forSuper: may benefit from a similar change.
>
> =============== Diff against Kernel-eem.1189 ===============
>
> Item was changed:
>   ----- Method: ClassBuilder>>reservedNames (in category 'private') -----
>   reservedNames
>         "Return a list of names that must not be used for variables"
> +       ^#(self super thisContext #true #false #nil)!
> -       ^#('self' 'super' 'thisContext' 'true' 'false' 'nil'
> -               self super thisContext #true #false #nil).!
>
> Item was changed:
>   ----- Method: ClassBuilder>>validateInstvars:from:forSuper: (in category 'validation') -----
> + validateInstvars: instVarArray from: oldClass forSuper: newSuper
> - validateInstvars: instVarArray from: oldClass forSuper: newSuper
>         "Check if any of the instVars of oldClass conflict with the new superclass"
> +       | usedNames |
> -       | instVars usedNames temp |
> -       instVarArray isEmpty ifTrue:[^true]. "Okay"
> -       newSuper allowsSubInstVars ifFalse: [
> -               self error: newSuper printString, ' does not allow subclass inst vars. See allowsSubInstVars.'. ^ false].
> -
> -       "Validate the inst var names"
>         usedNames := instVarArray asSet.
> +       oldClass ifNotNil: [ oldClass withAllSubclassesDo: [ : each | each == oldClass ifFalse: [usedNames addAll: each instVarNames] ] ].
> +       "Anything to possibly conflict?"
> +       usedNames isEmpty ifTrue: [ ^ true ].
> +       newSuper allowsSubInstVars ifFalse:
> +               [ self error: newSuper printString , ' does not allow subclass inst vars. See allowsSubInstVars.'.
> +               ^ false ].
> +       instVarArray asSet size = instVarArray size ifFalse:
> +               [ self error: instVarArray asBag sortedCounts first value, ' is multply defined.'.
> +               ^ false ].
> +       (usedNames intersection: self reservedNames) ifNotEmpty:
> +               [ : reservedWords | self error: reservedWords anyOne , ' is a reserved name'.
> +               ^ false ].
> +       newSuper ifNotNil:
> +               [ | offendingVars |
> +               "If any variable names in subclasses conflict names in the new superclass, report the offending class and instVar name."
> +               newSuper withAllSuperclasses
> +                       detect: [ : each | (offendingVars := each instVarNames intersection: usedNames) notEmpty ]
> +                       ifFound:
> +                               [ : offendingSuperclass |
> +                               DuplicateVariableError new
> +                                        superclass: offendingSuperclass ;
> +                                        variable: offendingVars anyOne ;
> +                                        signal: offendingVars anyOne , ' is already defined in ' , offendingSuperclass name ]
> +                       ifNone: [ "no name conflicts" ] ].
> +       ^ true!
> -       usedNames size = instVarArray size
> -               ifFalse:[       instVarArray do:[:var|
> -                                       usedNames remove: var ifAbsent:[temp := var]].
> -                               self error: temp,' is multiply defined'. ^false].
> -       (usedNames includesAnyOf: self reservedNames)
> -               ifTrue:[        self reservedNames do:[:var|
> -                                       (usedNames includes: var) ifTrue:[temp := var]].
> -                               self error: temp,' is a reserved name'. ^false].
> -
> -       newSuper == nil ifFalse:[
> -               usedNames := newSuper allInstVarNames asSet.
> -               instVarArray do:[:iv|
> -                       (usedNames includes: iv) ifTrue:[
> -                               newSuper withAllSuperclassesDo:[:cl|
> -                                       (cl instVarNames includes: iv) ifTrue:[temp := cl]].
> -                               (DuplicateVariableError new)
> -                                       superclass: temp;
> -                                       variable: iv;
> -                                       signal: iv,' is already defined in ', temp name]]].
> -       oldClass == nil ifFalse:[
> -               usedNames := Set new: 20.
> -               oldClass allSubclassesDo:[:cl| usedNames addAll: cl instVarNames].
> -               instVars := instVarArray.
> -               newSuper == nil ifFalse:[instVars := instVars, newSuper allInstVarNames].
> -               instVars do:[:iv|
> -                       (usedNames includes: iv) ifTrue:[
> -                               (DuplicateVariableError new)
> -                                       superclass: oldClass;
> -                                       variable: iv;
> -                                       signal: iv,' is already defined in a subclass of ', temp name]]].
> -       ^true!
>
>



--
_,,,^..^,,,_
best, Eliot


Reply | Threaded
Open this post in threaded view
|

Re: Why is testMoveVarFromSubToSuperclass doing that? (was: The Inbox: Kernel-cmm.1190.mcz)

Chris Muller-4
Hi Eliot,

>> But while working on that I was shocked to notice what
>> testMoveVarFromSubToSuperclass is doing and testing for.  Resuming the
>> DuplicateVariableException so it could, truly, make interrogations
>> about an object that shouldn't exist (right?).  Very strange!
>
>
> Imagine this:
>
> ... snip ...
>
> Does that explain the problem?

Yup, absolutely.  Thanks for the reminder!   :)

 - Chris