[KCP][ENH] KCP-0160-SystemChangeNotif (and related changesets - [et] mostly works (found 1 error), [er] I like it)

ducasse ducasse at iam.unibe.ch
Thu Jan 29 08:12:18 UTC 2004


On 28 janv. 04, at 23:11, Avi Bryant wrote:

> I'm posting a review on this thread, but in fact I tested several 
> related changesets.  Doug, if you need this separated out let me know, 
> but it's easier to review them as a group.
>
> Starting from 3.7a #5629, I loaded the following changesets, in order:
>
> KCP-0135-PrepareMovingChangesLog.cs
> KCP-0136-MovingChangesLog.cs
> KCP-0137-CleanUtilities.cs
> KCP-0147-moveUtilitiesEvaluateToCompiler.cs
> KCP-0148-moveBrowseUncommentedMethodToSystemNavigation.cs
> KCP-0160-SystemChangeNotif.cs
> KCP-0165-SystemChangeNotifHooks.2.cs
> KCP-0170-CompilerProtocolRefactoring.2.cs
>
> I tested various kinds of compiling, renaming, and removing, without 
> running into any errors, and checked to make sure that the changeset 
> recorded them properly.  This all seemed to work fine - I don't think 
> we're losing any changeset behavior we used to have.
>
> I did run into one problem when filing in an external file - at the 
> end of PositionableStream>>fileInAnnouncing: is "SystemNavigation 
> default forgetDoIts".  This causes a DNU.  Since it's also marked with 
> "self flag: #ThisMethodShouldNotBeThere", I wonder if someone simply 
> forgot to remove it.

no this is an old comment
The version two (KCP-0170-CompilerProtocolRefactoring.4.cs ) changes 
SystemNavigation default forgetDoIts into Smalltalk forgetDoIts.



>
> I read over the code for all of the changesets involved.  I don't have 
> intimate knowledge of the parts of the system involved, but as a user 
> of the affected protocols I was pleased with the changes.  The 
> SystemChangeNotification code will make Monticello's change tracking 
> *much* simpler - I had to resort to some ugly hacks in the past that 
> will no longer be necessary.  I also like the compiler protocol 
> refactoring.  For example, returning a compound CompiledMethodWithNode 
> object from #basicCompile:notifying:trailer:ifFail:  is easier to 
> understand and use than the somewhat baroque 
> #compile:notifying:trailer:ifFalse:elseSetSelectorAndNode: interface 
> it replaces.

:)

>   I'm also happy to have the installation of the method into the 
> method dictionary better separated from the compilation of the 
> CompiledMethod, and to have the notification as part of 
> #addSelector:withMethod: rather than as part of compilation.  One 
> thing that might be nice would be to further separate this out so that 
> adding the source to the .changes file wasn't done at the same time as 
> installing the method into the method dictionary (or could the 
> .changes file just be another client of the notifications?).

It is a client but we tight it together to have the same behavior as 
now.

>
> Another small note - it concerns me that #addSelector:withMethod: is 
> no longer sent by the normal compilation mechanisms, since people may 
> have overridden this - they would now need to override 
> #addSelector:withMethod:notifying:.  I don't know what the solution 
> should be, this may just be acceptable breakage.
>
> Avi
>
> On Jan 28, 2004, at 6:59 AM, n.schaerli at gmx.net wrote:
>
>> from preamble:
>>
>> "Change Set:		KCP-0160-SystemChangeNotif
>> Date:			28 January 2004
>> Author:			Roel Wuyts & Nathanael Schaerli
>>
>> New version of the system change notification framework. Working with
>> image v5623."!
>> <KCP-0160-SystemChangeNotif.cs.gz>
>
>




More information about the Squeak-dev mailing list