On Sat, Dec 15, 2001 at 05:56:19PM -0500, Nathan Froyd wrote:
> The attached patch removes all uses of SB-ITERATE:ITERATE from src/pcl.
> It doesn't eliminate src/pcl/iterate.lisp or the various support
> infrastructure for ITERATE (yet).
> GATHERING, WITH-GATHERING, and GATHERING1 have not been eliminated yet
> (although only 25 uses of them remain after this patch). I believe that
> in most of the cases, the code is clearer with the usage of them;
> nevertheless, I plan sometime this week to slog through and remove their
> usage as well (and eliminate src/pcl/iterate.lisp and various macros in
> Comments, critiques, criticisms, etc. welcome.
You wrote in a later message that you had removed the other
iterate.lisp stuff as well. Is that change sufficiently cleanly
separated from this one that it's easy for you to submit it as a
second patch, so that it's OK for me to merge this one as it stands?
Or would you prefer to submit all the goodbye-iterate.lisp stuff as a
Skimming over it it looks reasonable. However, I would ask that when
there's something which looks as though it can't be right, that you
put in not just a telegraphic comment, but a fairly complete
explanatory note, probably marked with a greppable tag like FIXME or
KLUDGE. So e.g. when you write
(defun make-dlap-lambda-list (metatypes applyp)
- (gathering1 (collecting)
- (iterate ((i (interval :from 0))
- (s (list-elements metatypes)))
- (progn s)
- (gather1 (dfun-arg-symbol i)))
+ (let ((lambda-list nil))
+ (dotimes (i (length metatypes))
+ (push (dfun-arg-symbol i) lambda-list))
- (gather1 '&rest))))
+ (push '&rest lambda-list)) ; where's the `.dfun-rest-arg.'?
+ (nreverse lambda-list)))
I'd encourage you instead to write something like
;; FIXME: This is a literal translation of the old
;; PCL code. It seems to work, but it's hard to see how
;; it can really be right, because &REST wants to be
;; followed by a .DFUN-REST-ARG. and I (NF 2001-12-15)
;; can't see where that would come from.
(push '&rest lambda-list))
I've been in that situation many times: the code works, and I need to
literally translate it or slightly modify it, but I can't understand
how it works, so it probably needs a bug fix or rewrite for clarity or
a better explanatory comment, or more than one of the above. My habit
is to write KLUDGE and FIXME notes like the one above, sometimes
throwing in guesses and stuff too:
;; Maybe the APPLYP case is dead code and can be deleted?
Usually I think it's a good idea. Ideally I'd wish that we wouldn't
check in weird confusing code, but sometimes -- especially when you're
maintaining old code -- it's hard to avoid. At that point, even if the
person checking it in doesn't understand the code, he likely
understands them better than anyone else on the project, and it's
worth preserving what he does understand in comments.
As a related example (written not by me but by one of the old PCL
implementors) from elsewhere in cache.lisp:
;;;; Its too bad Common Lisp compilers freak out when you have a
;;;; DEFUN with a lot of LABELS in it. If I could do that I could
;;;; make this code much easier to read and work with.
;;;; Ahh Scheme...
;;;; In the absence of that, the following little macro makes the
;;;; code that follows a little bit more reasonable. I would like to
;;;; add that having to practically write my own compiler in order to
;;;; get just this simple thing is something of a drag.
Ideally, we'd never check in this kind of code at all. But if we're
driven to it (by implementation limitations in this case, by being
overwhelmed by sheer mass of grotty old code in other cases, and
sometimes for other reasons too) it's good to explain it. Without this
kind of explanation, it'd be much more difficult to guess what's going
on in the code.
afterthought: Looking at the code more carefully in the
MAKE-DLAP-LAMBDA-LIST case, it appears that the answer is that it's
only called from EMIT-DEFAULT-ONLY, and EMIT-DEFAULT-ONLY has a
special hack to splice its differently-named &REST argument onto the
end. So I'd write something like
(push '&rest lambda-list)
;; KLUDGE: And "where's the .DFUN-REST-ARG.?", you might ask.
;; We're only called from EMIT-DEFAULT-ONLY, and it has a
;; special hack to splice its own &REST arg onto the end
;; here. -- WHN 2001-12-19
or if I was in a grot-must-die mood, I'd try to clean it up, perhaps
by generalizing the APPLYP calling convention throughout PCL (to
something like APPLYP-&REST, where a non-NIL value is used as the name
of the &REST argument), or perhaps by merging the
perhaps-too-closely-coupled-for-their-own-good EMIT-DEFAULT-ONLY and
MAKE-DLAP-LAMBDA-LIST into a single function, or perhaps just by
removing the APPLYP argument from MAKE-DLAP-LAMBDA-LIST and
making EMIT-DEFAULT-ONLY do something like
(let* ((dlap-lambda-list (make-dlap-lambda-list metatypes))
(args (remove '&rest dlap-lambda-list))
(restl (when applyp '(&rest .lap-rest-arg.))))
`(invoke-effective-method-function emf ,applyp ,@args ,@restl))))
except that that then gets into the rat's nest of what GENERATING-LISP
is doing exactly, ugh...
William Harold Newman <william.newman@...>
"Can't talk now. Must see Lord of the Rings, and then play more FFX. Is this a
great time to be alive, or what?" -- cmdrtaco on slashdot 2001-12-19
PGP key fingerprint 85 CE 1C BA 79 8D 51 8C B9 25 FB EE E0 C3 E5 7C