[KCP][ENH] KCP-0160-SystemChangeNotif (and related changesets -
[et] mostly works (found 1 error), [er] I like it)
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.
> Starting from 3.7a #5629, I loaded the following changesets, in order:
> 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.
> 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."!
More information about the Squeak-dev