[KCP][ENH] KCP-0160-SystemChangeNotif (and related changesets -
[et] mostly works (found 1 error), [er] I like it)
Avi Bryant
avi at beta4.com
Wed Jan 28 22:11:04 UTC 2004
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.
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
|