<div dir="ltr">Hi Chris,<br><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 29, 2018 at 7:33 PM Chris Muller <<a href="mailto:asqueaker@gmail.com">asqueaker@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Kernel-cmm.1190.mcz is my proposed fix for the bug identified by the<br>
new #testDuplicateInstanceVariableError, for your review.<br>
<br>
But while working on that I was shocked to notice what<br>
testMoveVarFromSubToSuperclass is doing and testing for. Resuming the<br>
DuplicateVariableException so it could, truly, make interrogations<br>
about an object that shouldn't exist (right?). Very strange!<br></blockquote><div><br></div><div>Imagine this:</div><div><br></div><div>Object subclass: #Fu</div><div> instanceVariableNames: ''.</div><div> </div><div>Fu subclass: #Bar</div><div> instanceVariableNames: 'fubar'.</div><div><br></div><div>and we want to refactor to this:</div><div><br></div><div>Object subclass: #Fu</div><div> instanceVariableNames: 'fubar'.</div><div> </div><div>Fu subclass: #Bar</div><div> instanceVariableNames: ''.</div><div><br></div><div>either we provide a class builder that can morph more than one class at a time or we have a problem.</div><div><br></div><div>If we refactor via this route:</div><div><br></div><div>1. { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar instanceVariableNames: 'fubar' }</div><div>=> { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar instanceVariableNames: '' }</div>=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar instanceVariableNames: '' }</div><div class="gmail_quote">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</div><div class="gmail_quote"><br></div><div class="gmail_quote">If we refactor via this route:</div><div class="gmail_quote">2.<br><div>1. { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar instanceVariableNames: 'fubar' }</div><div>=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar instanceVariableNames: 'fubar' }</div>=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar instanceVariableNames: '' }<div><br></div><div>we have a DuplicateVariableException to handle, but state could conceivably be preserved across the refactoring.</div><div><br></div><div>Does that explain the problem?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Best,<br>
Chris<br>
<br>
On Wed, Aug 29, 2018 at 9:20 PM <<a href="mailto:commits@source.squeak.org" target="_blank">commits@source.squeak.org</a>> wrote:<br>
><br>
> Chris Muller uploaded a new version of Kernel to project The Inbox:<br>
> <a href="http://source.squeak.org/inbox/Kernel-cmm.1190.mcz" rel="noreferrer" target="_blank">http://source.squeak.org/inbox/Kernel-cmm.1190.mcz</a><br>
><br>
> ==================== Summary ====================<br>
><br>
> Name: Kernel-cmm.1190<br>
> Author: cmm<br>
> Time: 29 August 2018, 9:20:01.027647 pm<br>
> UUID: b5a78e5d-c7fe-4e5d-9673-ee6208534ae1<br>
> Ancestors: Kernel-eem.1189<br>
><br>
> - Remove unnecessary string literals from ClassBuilder>>#reservedNames.<br>
> - Fix and simplify ClassBuilder>>#validateInstvars:from:forSuper:.<br>
> ClassBuilder>>#validateClassvars:from:forSuper: may benefit from a similar change.<br>
><br>
> =============== Diff against Kernel-eem.1189 ===============<br>
><br>
> Item was changed:<br>
> ----- Method: ClassBuilder>>reservedNames (in category 'private') -----<br>
> reservedNames<br>
> "Return a list of names that must not be used for variables"<br>
> + ^#(self super thisContext #true #false #nil)!<br>
> - ^#('self' 'super' 'thisContext' 'true' 'false' 'nil'<br>
> - self super thisContext #true #false #nil).!<br>
><br>
> Item was changed:<br>
> ----- Method: ClassBuilder>>validateInstvars:from:forSuper: (in category 'validation') -----<br>
> + validateInstvars: instVarArray from: oldClass forSuper: newSuper<br>
> - validateInstvars: instVarArray from: oldClass forSuper: newSuper<br>
> "Check if any of the instVars of oldClass conflict with the new superclass"<br>
> + | usedNames |<br>
> - | instVars usedNames temp |<br>
> - instVarArray isEmpty ifTrue:[^true]. "Okay"<br>
> - newSuper allowsSubInstVars ifFalse: [<br>
> - self error: newSuper printString, ' does not allow subclass inst vars. See allowsSubInstVars.'. ^ false].<br>
> -<br>
> - "Validate the inst var names"<br>
> usedNames := instVarArray asSet.<br>
> + oldClass ifNotNil: [ oldClass withAllSubclassesDo: [ : each | each == oldClass ifFalse: [usedNames addAll: each instVarNames] ] ].<br>
> + "Anything to possibly conflict?"<br>
> + usedNames isEmpty ifTrue: [ ^ true ].<br>
> + newSuper allowsSubInstVars ifFalse:<br>
> + [ self error: newSuper printString , ' does not allow subclass inst vars. See allowsSubInstVars.'.<br>
> + ^ false ].<br>
> + instVarArray asSet size = instVarArray size ifFalse:<br>
> + [ self error: instVarArray asBag sortedCounts first value, ' is multply defined.'.<br>
> + ^ false ].<br>
> + (usedNames intersection: self reservedNames) ifNotEmpty:<br>
> + [ : reservedWords | self error: reservedWords anyOne , ' is a reserved name'.<br>
> + ^ false ].<br>
> + newSuper ifNotNil:<br>
> + [ | offendingVars |<br>
> + "If any variable names in subclasses conflict names in the new superclass, report the offending class and instVar name."<br>
> + newSuper withAllSuperclasses<br>
> + detect: [ : each | (offendingVars := each instVarNames intersection: usedNames) notEmpty ]<br>
> + ifFound:<br>
> + [ : offendingSuperclass |<br>
> + DuplicateVariableError new<br>
> + superclass: offendingSuperclass ;<br>
> + variable: offendingVars anyOne ;<br>
> + signal: offendingVars anyOne , ' is already defined in ' , offendingSuperclass name ]<br>
> + ifNone: [ "no name conflicts" ] ].<br>
> + ^ true!<br>
> - usedNames size = instVarArray size<br>
> - ifFalse:[ instVarArray do:[:var|<br>
> - usedNames remove: var ifAbsent:[temp := var]].<br>
> - self error: temp,' is multiply defined'. ^false].<br>
> - (usedNames includesAnyOf: self reservedNames)<br>
> - ifTrue:[ self reservedNames do:[:var|<br>
> - (usedNames includes: var) ifTrue:[temp := var]].<br>
> - self error: temp,' is a reserved name'. ^false].<br>
> -<br>
> - newSuper == nil ifFalse:[<br>
> - usedNames := newSuper allInstVarNames asSet.<br>
> - instVarArray do:[:iv|<br>
> - (usedNames includes: iv) ifTrue:[<br>
> - newSuper withAllSuperclassesDo:[:cl|<br>
> - (cl instVarNames includes: iv) ifTrue:[temp := cl]].<br>
> - (DuplicateVariableError new)<br>
> - superclass: temp;<br>
> - variable: iv;<br>
> - signal: iv,' is already defined in ', temp name]]].<br>
> - oldClass == nil ifFalse:[<br>
> - usedNames := Set new: 20.<br>
> - oldClass allSubclassesDo:[:cl| usedNames addAll: cl instVarNames].<br>
> - instVars := instVarArray.<br>
> - newSuper == nil ifFalse:[instVars := instVars, newSuper allInstVarNames].<br>
> - instVars do:[:iv|<br>
> - (usedNames includes: iv) ifTrue:[<br>
> - (DuplicateVariableError new)<br>
> - superclass: oldClass;<br>
> - variable: iv;<br>
> - signal: iv,' is already defined in a subclass of ', temp name]]].<br>
> - ^true!<br>
><br>
><br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div></div>