A new version of Kernel was added to project The Inbox:
http://source.squeak.org/inbox/Kernel-cmm.830.mcz ==================== Summary ==================== Name: Kernel-cmm.830 Author: cmm Time: 7 January 2014, 9:31:35.206 pm UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed Ancestors: Kernel-fbs.829 Removing a class should remove its subclasses too. =============== Diff against Kernel-fbs.829 =============== Item was changed: ----- Method: Class>>removeFromSystem: (in category 'initialize-release') ----- removeFromSystem: logged "Forget the receiver from the Smalltalk global dictionary. Any existing instances will refer to an obsolete version of the receiver." "keep the class name and category for triggering the system change message. If we wait to long, then we get obsolete information which is not what we want." "tell class to deactivate and unload itself-- two separate events in the module system" + self subclassesDo: [ : each | each removeFromSystem: logged ]. self deactivate; unload. self superclass ifNotNil: ["If we have no superclass there's nothing to be remembered" self superclass addObsoleteSubclass: self]. self environment forgetClass: self logged: logged. self obsolete.! |
Command+X on TestCase in the browser. Sweet. :)
On Tue, Jan 7, 2014 at 9:31 PM, <[hidden email]> wrote: > A new version of Kernel was added to project The Inbox: > http://source.squeak.org/inbox/Kernel-cmm.830.mcz > > ==================== Summary ==================== > > Name: Kernel-cmm.830 > Author: cmm > Time: 7 January 2014, 9:31:35.206 pm > UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed > Ancestors: Kernel-fbs.829 > > Removing a class should remove its subclasses too. > > =============== Diff against Kernel-fbs.829 =============== > > Item was changed: > ----- Method: Class>>removeFromSystem: (in category 'initialize-release') ----- > removeFromSystem: logged > "Forget the receiver from the Smalltalk global dictionary. Any existing > instances will refer to an obsolete version of the receiver." > > "keep the class name and category for triggering the system change message. If we wait to long, then we get obsolete information which is not what we want." > > "tell class to deactivate and unload itself-- two separate events in the module system" > + self subclassesDo: [ : each | each removeFromSystem: logged ]. > self deactivate; unload. > self superclass ifNotNil: > ["If we have no superclass there's nothing to be remembered" > self superclass addObsoleteSubclass: self]. > self environment forgetClass: self logged: logged. > self obsolete.! > > |
On 08 Jan 2014, at 3:36, Chris Muller <[hidden email]> wrote:
> Command+X on TestCase in the browser. Sweet. :) > > On Tue, Jan 7, 2014 at 9:31 PM, <[hidden email]> wrote: >> A new version of Kernel was added to project The Inbox: >> http://source.squeak.org/inbox/Kernel-cmm.830.mcz >> >> ==================== Summary ==================== >> >> Name: Kernel-cmm.830 >> Author: cmm >> Time: 7 January 2014, 9:31:35.206 pm >> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed >> Ancestors: Kernel-fbs.829 >> >> Removing a class should remove its subclasses too. >> >> =============== Diff against Kernel-fbs.829 =============== >> >> Item was changed: >> ----- Method: Class>>removeFromSystem: (in category 'initialize-release') ----- >> removeFromSystem: logged >> "Forget the receiver from the Smalltalk global dictionary. Any existing >> instances will refer to an obsolete version of the receiver." >> >> "keep the class name and category for triggering the system change message. If we wait to long, then we get obsolete information which is not what we want." >> >> "tell class to deactivate and unload itself-- two separate events in the module system" >> + self subclassesDo: [ : each | each removeFromSystem: logged ]. I'd rather see this as a separate selector, maybe #removeFromSystemRecursively: or #removeFromSystemWithSubclasses or something:. Imagine you're unloading a package. Often there will be several 'root' classes in your package: a base domain object, a base exception, other bits. How can I write a package unloader to unload your package? Either I have to understand your package structure, or I end up calling #removeFromSystem: multiple times for non-root classes (because MCClassDefinition will do this when it unloads). I'd be happy to see it used via cmd-X, perhaps with certain guards, or a confirmation saying "this will remove Foo, SubFoo, SubSubFoo and FooNotification. Are you sure?" frank >> self deactivate; unload. >> self superclass ifNotNil: >> ["If we have no superclass there's nothing to be remembered" >> self superclass addObsoleteSubclass: self]. >> self environment forgetClass: self logged: logged. >> self obsolete.! >> >> > |
In reply to this post by commits-2
On 08.01.2014, at 03:31, [hidden email] wrote: > A new version of Kernel was added to project The Inbox: > http://source.squeak.org/inbox/Kernel-cmm.830.mcz > > ==================== Summary ==================== > > Name: Kernel-cmm.830 > Author: cmm > Time: 7 January 2014, 9:31:35.206 pm > UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed > Ancestors: Kernel-fbs.829 > > Removing a class should remove its subclasses too. > > =============== Diff against Kernel-fbs.829 =============== > > Item was changed: > ----- Method: Class>>removeFromSystem: (in category 'initialize-release') ----- > removeFromSystem: logged > "Forget the receiver from the Smalltalk global dictionary. Any existing > instances will refer to an obsolete version of the receiver." That means, the subclasses will be subclasses from the obsoleted class. > "keep the class name and category for triggering the system change message. If we wait to long, then we get obsolete information which is not what we want." > > "tell class to deactivate and unload itself-- two separate events in the module system" > + self subclassesDo: [ : each | each removeFromSystem: logged ]. > self deactivate; unload. > self superclass ifNotNil: > ["If we have no superclass there's nothing to be remembered" > self superclass addObsoleteSubclass: self]. > self environment forgetClass: self logged: logged. > self obsolete.! > > signature.asc (1K) Download Attachment |
>> A new version of Kernel was added to project The Inbox:
>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz >> >> ==================== Summary ==================== >> >> Name: Kernel-cmm.830 >> Author: cmm >> Time: 7 January 2014, 9:31:35.206 pm >> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed >> Ancestors: Kernel-fbs.829 >> >> Removing a class should remove its subclasses too. >> > > No. Why? > See comment Ok, so if I remove the comment we're golden..? Otherwise, I want to understand why I would ever want to have a class with an ObsoleteSuperclass..? Because I think the system should cater to regular Smalltalk development. "Regular" meaning, all classes have a valid superclass. :) |
On 08.01.2014, at 16:34, Chris Muller <[hidden email]> wrote: >>> A new version of Kernel was added to project The Inbox: >>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz >>> >>> ==================== Summary ==================== >>> >>> Name: Kernel-cmm.830 >>> Author: cmm >>> Time: 7 January 2014, 9:31:35.206 pm >>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed >>> Ancestors: Kernel-fbs.829 >>> >>> Removing a class should remove its subclasses too. >>> >> >> No. > > Why? > >> See comment > > Ok, so if I remove the comment we're golden..? > > Otherwise, I want to understand why I would ever want to have a class > with an ObsoleteSuperclass..? > > Because I think the system should cater to regular Smalltalk > development. "Regular" meaning, all classes have a valid superclass. I fear that there is functionality in the system that depends on being-removed classes not removing their subclasses. However, I am not sure. Can one of the older (in the sense of wiser) guys chime in here? If we know this cannot happen, I’m fine with the change. Best -Tobias signature.asc (1K) Download Attachment |
On 8 January 2014 15:38, Tobias Pape <[hidden email]> wrote:
> > On 08.01.2014, at 16:34, Chris Muller <[hidden email]> wrote: > >>>> A new version of Kernel was added to project The Inbox: >>>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz >>>> >>>> ==================== Summary ==================== >>>> >>>> Name: Kernel-cmm.830 >>>> Author: cmm >>>> Time: 7 January 2014, 9:31:35.206 pm >>>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed >>>> Ancestors: Kernel-fbs.829 >>>> >>>> Removing a class should remove its subclasses too. >>>> >>> >>> No. >> >> Why? >> >>> See comment >> >> Ok, so if I remove the comment we're golden..? >> >> Otherwise, I want to understand why I would ever want to have a class >> with an ObsoleteSuperclass..? >> >> Because I think the system should cater to regular Smalltalk >> development. "Regular" meaning, all classes have a valid superclass. > > Ok, let me rephrase: > I fear that there is functionality in the system that depends on > being-removed classes not removing their subclasses. > However, I am not sure. > > Can one of the older (in the sense of wiser) guys chime in here? I'm not older or wiser, but if we remove subclasses when we remove classes, unloading an MCClassDefinition will result in a bunch of MCClassDefinition's backing classes disappearing out from under them. As a package unloads, each subclass will be #unload'd N times, N being its depth in the inheritance hierarchy (where the root's the argument to the method). That's why while I'm happy with the _feature_, I want to see it as a _new_ feature, leaving #removeFromSystem: removing just a single class. frank > If we know this cannot happen, I’m fine with the change. > > Best > -Tobias > > > > |
On Wed, Jan 8, 2014 at 9:45 AM, Frank Shearar <[hidden email]> wrote:
> On 8 January 2014 15:38, Tobias Pape <[hidden email]> wrote: >> >> On 08.01.2014, at 16:34, Chris Muller <[hidden email]> wrote: >> >>>>> A new version of Kernel was added to project The Inbox: >>>>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz >>>>> >>>>> ==================== Summary ==================== >>>>> >>>>> Name: Kernel-cmm.830 >>>>> Author: cmm >>>>> Time: 7 January 2014, 9:31:35.206 pm >>>>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed >>>>> Ancestors: Kernel-fbs.829 >>>>> >>>>> Removing a class should remove its subclasses too. >>>>> >>>> >>>> No. >>> >>> Why? >>> >>>> See comment >>> >>> Ok, so if I remove the comment we're golden..? >>> >>> Otherwise, I want to understand why I would ever want to have a class >>> with an ObsoleteSuperclass..? >>> >>> Because I think the system should cater to regular Smalltalk >>> development. "Regular" meaning, all classes have a valid superclass. >> >> Ok, let me rephrase: >> I fear that there is functionality in the system that depends on >> being-removed classes not removing their subclasses. >> However, I am not sure. >> >> Can one of the older (in the sense of wiser) guys chime in here? > > I'm not older or wiser, but if we remove subclasses when we remove > classes, unloading an MCClassDefinition will result in a bunch of > MCClassDefinition's backing classes disappearing out from under them. > As a package unloads, each subclass will be #unload'd N times, N being > its depth in the inheritance hierarchy (where the root's the argument > to the method). Browse MCClassDefinition>>#unload and trace it into SystemDictionary>>#removeClassNamed:. There's a guard to ensure it's still there before trying to unload it. So it's just N lookups in a Dictionary, it won't be unloaded N times. Nevertheless, I factored the individual remove as #primRemoveFromSystem:, just for you. :) So if some external caller wants to use that they can (although doing it seems it leaves the system in a bad state, maybe there's some use case I'm missing). |
On Wed, Jan 8, 2014 at 11:29 AM, Chris Muller <[hidden email]> wrote:
All this environments and shrinking work I've been doing lately has made me aware of just how complicated and delicate class removal really is. I've been thinking we should do some work on simplifying it in 4.6.
It may be that this *should* be the default behaviour, but I'm with Tobias, we shouldn't change it lightly. If you want to release 4.5 soon, I don't think there's enough time to test a change this fundamental.
Also, a nit: since it's not really a primitive, #primRemoveFromSystem: should be called #basicRemoveFromSystem: Colin |
In reply to this post by Chris Muller-3
On 8 January 2014 16:29, Chris Muller <[hidden email]> wrote:
> On Wed, Jan 8, 2014 at 9:45 AM, Frank Shearar <[hidden email]> wrote: >> On 8 January 2014 15:38, Tobias Pape <[hidden email]> wrote: >>> >>> On 08.01.2014, at 16:34, Chris Muller <[hidden email]> wrote: >>> >>>>>> A new version of Kernel was added to project The Inbox: >>>>>> http://source.squeak.org/inbox/Kernel-cmm.830.mcz >>>>>> >>>>>> ==================== Summary ==================== >>>>>> >>>>>> Name: Kernel-cmm.830 >>>>>> Author: cmm >>>>>> Time: 7 January 2014, 9:31:35.206 pm >>>>>> UUID: 43453587-ee4b-48e4-93ec-43983f0c82ed >>>>>> Ancestors: Kernel-fbs.829 >>>>>> >>>>>> Removing a class should remove its subclasses too. >>>>>> >>>>> >>>>> No. >>>> >>>> Why? >>>> >>>>> See comment >>>> >>>> Ok, so if I remove the comment we're golden..? >>>> >>>> Otherwise, I want to understand why I would ever want to have a class >>>> with an ObsoleteSuperclass..? >>>> >>>> Because I think the system should cater to regular Smalltalk >>>> development. "Regular" meaning, all classes have a valid superclass. >>> >>> Ok, let me rephrase: >>> I fear that there is functionality in the system that depends on >>> being-removed classes not removing their subclasses. >>> However, I am not sure. >>> >>> Can one of the older (in the sense of wiser) guys chime in here? >> >> I'm not older or wiser, but if we remove subclasses when we remove >> classes, unloading an MCClassDefinition will result in a bunch of >> MCClassDefinition's backing classes disappearing out from under them. >> As a package unloads, each subclass will be #unload'd N times, N being >> its depth in the inheritance hierarchy (where the root's the argument >> to the method). So I should really check ALL my mail before responding. > Browse MCClassDefinition>>#unload and trace it into > SystemDictionary>>#removeClassNamed:. There's a guard to ensure it's > still there before trying to unload it. It won't route through SystemDictionary because there are no instances of SystemDictionary in the base image. But yes, Environment >> #removeClassNamed: does guard. > So it's just N lookups in a Dictionary, it won't be unloaded N times. That doesn't make me any happier :) OK, so in my other mail my worry of multiple #unload sends won't come to pass, but the N lookups for the _usual_ code path just smells bad. I don't see why we shouldn't keep just-this-class as the default behaviour - it's really quite deep in the bowels of the system - and keep yours & Colin's unload-em-all as a _user level_ optimisation. It's great for the _user_ to be able to do this. > Nevertheless, I factored the individual remove as > #primRemoveFromSystem:, just for you. :) So if some external caller > wants to use that they can (although doing it seems it leaves the > system in a bad state, maybe there's some use case I'm missing). Yeah, as I said I would much rather see it the other way round. frank |
On Wed, Jan 8, 2014 at 1:35 PM, Frank Shearar <[hidden email]> wrote:
Mine? No... I'm willing to listen to the argument, but so far I haven't heard one. And even given a irrefutable arguments, I'd say wait until 4.6. |
In reply to this post by Colin Putney-3
>> Nevertheless, I factored the individual remove as
>> #primRemoveFromSystem:, just for you. :) So if some external caller >> wants to use that they can (although doing it seems it leaves the >> system in a bad state, maybe there's some use case I'm missing). > > All this environments and shrinking work I've been doing lately has made me > aware of just how complicated and delicate class removal really is. I've > been thinking we should do some work on simplifying it in 4.6. One thing that's true is, I have no experience with shrinking, so, that's fine. There's no hurry on this. :) > It may be that this *should* be the default behaviour, but I'm with Tobias, > we shouldn't change it lightly. If you want to release 4.5 soon, I don't > think there's enough time to test a change this fundamental. However, I do think that the image is basically nowhere in terms of deleting classes. IOW, no one ever deletes classes with subclasses except during development. > Also, a nit: since it's not really a primitive, #primRemoveFromSystem: > should be called #basicRemoveFromSystem: Sure, that's fine. I just wanted it to be something that connotates being at a "lower-level" than the API. #basic does that just fine. |
In reply to this post by Colin Putney-3
On 8 January 2014 20:05, Colin Putney <[hidden email]> wrote:
> > > > On Wed, Jan 8, 2014 at 1:35 PM, Frank Shearar <[hidden email]> > wrote: > >> >> That doesn't make me any happier :) OK, so in my other mail my worry >> of multiple #unload sends won't come to pass, but the N lookups for >> the _usual_ code path just smells bad. I don't see why we shouldn't >> keep just-this-class as the default behaviour - it's really quite deep >> in the bowels of the system - and keep yours & Colin's unload-em-all >> as a _user level_ optimisation. It's great for the _user_ to be able >> to do this. > > > Mine? No... I'm willing to listen to the argument, but so far I haven't > heard one. Sure you have: "there is no sensible use case for deleting a class when it has subclasses: you leave the inheritance hierarchy broken". > And even given a irrefutable arguments, I'd say wait until 4.6. Why did I think you'd actually done that in the shrink.st? You don't! Yeah, ok. But strongly agree re "wait until 4.6". frank |
Free forum by Nabble | Edit this page |