hi Andreas,
long time no see :-) rw is me.
I had a quick look at the method in the RC2 image. I added a trigger in this method for the change mechanism to tell: * whether the category of a class changed, * whether the class definition changed, * or when it is a new class that was added.
The implementation checks for these occurences and is validated by the corresponding unit tests.
Do you think there is problems with it ?
Begin forwarded message:
From: Andreas Raab andreas.raab@gmx.de Date: 23 octobre 2006 20:40:33 HAEC To: The general-purpose Squeak developers list <squeak- dev@lists.squeakfoundation.org> Subject: ClassBuilder changes (was: Re: Navigating traits in 3.9) Reply-To: The general-purpose Squeak developers list <squeak- dev@lists.squeakfoundation.org>
Trying to identify the problem made me stumble into ClassBuilder and looking at the core method ClassBuilder>>name: className inEnvironment: env subclassOf: newSuper type: type instanceVariableNames: instVarString classVariableNames: classVarString poolDictionaries: poolString category: category unsafe: unsafe.
This makes me wonder what the intend of the latest changes there were. They do look rather suspicious to me. Could someone with the initials "rw" please explain the reasoning for these changes? It's the last person who touched that method and unfortunately there is no method history any longer :-((( (the version found in 3.8 from '04 looks approximately correct so any changes inbetween would be interesting)
Cheers,
- Andreas
Andreas Raab wrote:
Klaus D. Witzel wrote:
on Mon, 23 Oct 2006 10:35:13 +0200, you wrote:
... Looking at TCompilingBehavior's users I find TPureBehavior; looking at TCompilingDescription's users I find ... "an IdentitySet(ClassDescription ClassDescription TraitDescription)". Huh? Anyone else seeing this? An identity set with a duplicate entry?
Confirmed, printing "array fourth == array fifth" in an inspector on TCompilingDescription => true.
And not only that. All of the following traits have duplicate users and it's not limited to ClassDescription/TraitDescription either. TAccessingMethodDictDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription) TBasicCategorisingDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription) TBehaviorCategorization users an IdentitySet(Trait Class Class Class) TCommentDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription) TCompilingDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription) TCopyingDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription) TFileInOutDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription) TPrintingDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription) TPureBehavior users an IdentitySet(TraitBehavior Behavior Behavior) TTestingDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription) TTraitsCategorisingDescription users an IdentitySet(ClassDescription ClassDescription TraitDescription)
Hi Roel -
Roel Wuyts wrote:
long time no see :-) rw is me.
Ah yes, should have thought of that.
The implementation checks for these occurences and is validated by the corresponding unit tests.
Do you think there is problems with it?
I am suspecting there is an issue with traits registration due to the copy of the original class you are creating in ClassBuilder. Which seems to be required by the notification mechanism but strikes me as conflicting by definition, since it asks for both, the old *and* the new class to be accessible at the same time. Which, by virtue of the invariants guaranteed in ClassBuilder, should never ever happen (and I am VERY suspicious of just throwing incomplete copies of classes around).
Question: Wouldn't it be more logical, less conflicting and just generally better design to drop the requirement of having both classes accessible and instead simply use a ClassDefinition object for the old class? This may look a lot like a class except that it isn't a behavior and therefore not subject to ClassBuilder invariants.
Cheers, - Andreas
Hi Andreas,
that part of the code was not me, I think.
For the change notification I only asumed to get the *name* of a renamed or removed element to be passed around (whether a class or a method or whatever else), not the removed/renamed element itself (for the reasons you mention). But the version in the image is not exactly mine (whoever put it in there made some changes for better or for worse), and this version does not seem to be from me. But ok...
I can fix this, I guess (I think that Nathanael was the maintainer for the class builder, but I do not know whether he is still around for this)...
You need this urgently, I assume ?
PS: Can you reply personally to me as well ? I do not follow the squeak list as often as I should :-( But I do fix things that get included in the releases :-)
On 24 Oct 2006, at 10:09, Andreas Raab wrote:
Hi Roel -
Roel Wuyts wrote:
long time no see :-) rw is me.
Ah yes, should have thought of that.
The implementation checks for these occurences and is validated by the corresponding unit tests. Do you think there is problems with it?
I am suspecting there is an issue with traits registration due to the copy of the original class you are creating in ClassBuilder. Which seems to be required by the notification mechanism but strikes me as conflicting by definition, since it asks for both, the old *and* the new class to be accessible at the same time. Which, by virtue of the invariants guaranteed in ClassBuilder, should never ever happen (and I am VERY suspicious of just throwing incomplete copies of classes around).
Question: Wouldn't it be more logical, less conflicting and just generally better design to drop the requirement of having both classes accessible and instead simply use a ClassDefinition object for the old class? This may look a lot like a class except that it isn't a behavior and therefore not subject to ClassBuilder invariants.
Cheers,
- Andreas
Hi Roel -
Roel Wuyts wrote:
For the change notification I only asumed to get the *name* of a renamed or removed element to be passed around (whether a class or a method or whatever else), not the removed/renamed element itself (for the reasons you mention). But the version in the image is not exactly mine (whoever put it in there made some changes for better or for worse), and this version does not seem to be from me. But ok...
Yeah, it's quite possible. Like I said since the history is gone so I can't be sure who changed what but since your initials were the last I thought you might know what these changes are intended for.
I can fix this, I guess (I think that Nathanael was the maintainer for the class builder, but I do not know whether he is still around for this)...
You need this urgently, I assume ?
I really don't, since I don't use 3.9 at all. It just seems "broken by design" and I was curious if this is what's causing the traits problem.
PS: Can you reply personally to me as well ? I do not follow the squeak list as often as I should :-( But I do fix things that get included in the releases :-)
Sure, can do that. I just don't do it generally unless people ask me because personally, I'm just the opposite ;-)
Cheers, - Andreas
I really don't, since I don't use 3.9 at all. It just seems "broken by design" and I was curious if this is what's causing the traits problem.
I am using 3.9 for all serious (industrial) work. It works well by design, without having to apply hundreds of patches manually before being productive.
The only exception is the Graphics/Balloon bug that I reported here: http://bugs.impara.de/view.php?id=5222
Roel, I would really appreciate if this bug in the ClassBuilder/ChangeNotification could be fixed.
Cheers, Lukas
Lukas Renggli wrote:
I am using 3.9 for all serious (industrial) work. It works well by design, without having to apply hundreds of patches manually before being productive.
With "it" being broken by design I meant the exposure of the "before" and "after" version of a class in the notification mechanism, not 3.9 in general. Sorry for the confusing choice of words.
Cheers, - Andreas
Hi everyone,
I wanted to note that Andrew Black and I have also touched the change notification integration into class builder last summer during the Traits work.
The story as I remember it - the traits work required massive recreation of classes due to adding variables somewhere around behavior. It turned out that this corrupted the subclasses/superclass relationship, and we traced the bug to the code you're looking at. If I remember correctly, the problem was that for SCN, CB creates a copy of the class, but this was not always cleaned up because CB uses an early return to signal failure. IIRC, my fix was to add the send of #ensure: you can see is now there.
I didn't, however, add the copying of the class itself - that was already there.
To move forward - anyone else been touching that code, and possibly remembers what the copies were added for? AFAICT, ModifiedClassDefinitionEvent does indeed keep copies of the new and old definitions, and from looking at their uses, I think Andreas' suggestion of a "record object" would probably do just fine. At a second look, I would guess that the use of copies of the old class dates to the addition of the "anyChanges" method, apparently by NathanaelS.
Daniel Vainsencher
Roel Wuyts wrote:
Hi Andreas,
that part of the code was not me, I think.
For the change notification I only asumed to get the *name* of a renamed or removed element to be passed around (whether a class or a method or whatever else), not the removed/renamed element itself (for the reasons you mention). But the version in the image is not exactly mine (whoever put it in there made some changes for better or for worse), and this version does not seem to be from me. But ok...
I can fix this, I guess (I think that Nathanael was the maintainer for the class builder, but I do not know whether he is still around for this)...
You need this urgently, I assume ?
PS: Can you reply personally to me as well ? I do not follow the squeak list as often as I should :-( But I do fix things that get included in the releases :-)
On 24 Oct 2006, at 10:09, Andreas Raab wrote:
Hi Roel -
Roel Wuyts wrote:
long time no see :-) rw is me.
Ah yes, should have thought of that.
The implementation checks for these occurences and is validated by the corresponding unit tests. Do you think there is problems with it?
I am suspecting there is an issue with traits registration due to the copy of the original class you are creating in ClassBuilder. Which seems to be required by the notification mechanism but strikes me as conflicting by definition, since it asks for both, the old *and* the new class to be accessible at the same time. Which, by virtue of the invariants guaranteed in ClassBuilder, should never ever happen (and I am VERY suspicious of just throwing incomplete copies of classes around).
Question: Wouldn't it be more logical, less conflicting and just generally better design to drop the requirement of having both classes accessible and instead simply use a ClassDefinition object for the old class? This may look a lot like a class except that it isn't a behavior and therefore not subject to ClassBuilder invariants.
Cheers,
- Andreas
squeak-dev@lists.squeakfoundation.org
-
Andreas Raab -
Daniel Vainsencher -
Lukas Renggli -
Roel Wuyts