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

Doug Way dway at mailcan.com
Thu Jan 29 02:58:42 UTC 2004


On Jan 28, 2004, at 5:11 PM, 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.

Reviewing as a group is fine... thanks for the detailed review.

Actually I think it's important that written reviews of bigger changes 
are posted to squeak-dev and not squeak-harvest... it might be nice to 
somehow encourage that in the BFAV UI.

- Doug


> 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.
>
> 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?).
>
> 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