danielv at netvision.net.il
Sun May 4 23:05:08 UTC 2003
Ok, I've gone through the KCP stuff. I know we orginally decided to do
the removals first, but I had an urge, and this doesn't touch so many
different classes that I'm very worried about clashes. Anyway, it seems
as the designated reviewer for this specific project I got back from my
vacation before the removals were done, and I don't see much reason not
to merge these now, but I don't mind checking the conflicts if this
proposal isn't accepted.
I've reviewed almost all the code changes, but didn't test extensively.
I did find one bug described below, but we can easily fix small bugs as
we find them, I'm mostly reviewing the design decision, which seems very
sound. In a few places a little more intent is warranted, because we
will actually want people to continue in the new direction, so I asked
Anyway, everything currently posted (and enumerated explicitly below,
since web pages change), is approved.
Loads of thanks to the KCP team - hopefully we will soon see the kernel
just be a kernel.
Bug - ctrl-w doesn't work because
SystemNavigator>>allSelectorsWithAnyImplementorsIn: tries to get a
system navigator. Maybe a quick review of all the query methods is in
order, to see they don't have such stuff.
Behavior>>becomeCompact has the line (also a similar method)
cct _ self environment compactClassesArray.
Since you're taking SystemDictionary apart (woohoo!) be careful to then
also fix all these references you change to self environment. I'm not
sure this isn't making things more difficult for you, but anyway, you
can later look for environment senders, I guess.
About SystemNavigation - it has a nice comment that does answer some of
the questions I had, but can you say something about how it's supposed
to fit into everything? for example, why all the similar implementations
of self systemNavigator and not one implementation in Object (wait!
don't shoot!) in a class extension? it's not very clear in what case
you'd need state for this bunch of utility methods, so what examples did
you have in mind when you made them instance side?
With all the deprecation going on, is there a test that finds all
deprecated methods, finds all their senders, and makes sure it's 0, or
that the senders are to self environment or self navigator? seems to me
safer than all the "this should be deprecated" tests, because one of
these tests might be missing. It seems a bit more complicated than this,
because some are not renamed, just moved, but I think some partial
sanity checks of this type might be good.
13, 14, 16, 18, 19, 21: approved. Thank you for moving to the
self-sufficient-changeset style (where deprecation of a method and it's
redefinition in SystemNavigation are in the same changeset). It's much
easier to check.
22, 24, 26, 28, 30, 32, 34, 38, 40, 41, 43, 45, 46, 48 50 52, 55 and 57
More information about the Squeak-dev