Re: [Sbcl-devel] [PATCH] Goodbye SB-ITERATE:ITERATE usage

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
> src/pcl/macros.lisp).
>
> Comments, critiques, criticisms, etc. welcome.
--
a question
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
single patch?
--
a comment
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))
(when applyp
- (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.
(when applyp
(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
(when applyp
(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.))))
(generating-lisp '(emf)
dlap-lambda-list
`(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

Thread view

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
src/pcl/macros.lisp).
Comments, critiques, criticisms, etc. welcome.
--
Nathan | http://www.rose-hulman.edu/~froydnj/ | veritas aeterna
Yes, God had a deadline. So He wrote it all in Lisp.
Lisp. Everything else is just Turing complete.

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
> src/pcl/macros.lisp).
>
> Comments, critiques, criticisms, etc. welcome.
--
a question
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
single patch?
--
a comment
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))
(when applyp
- (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.
(when applyp
(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
(when applyp
(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.))))
(generating-lisp '(emf)
dlap-lambda-list
`(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

On Wed, 2001-12-19 at 15:38, William Harold Newman wrote:
> 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).
> --
> a question
>
> 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
> single patch?
The no-gathering patch touches:
construct.lisp
defclass.lisp
defcombin.lisp
dfun.lisp
fast-init.lisp
fngen.lisp
macros.lisp
std-class.lisp
vector.lisp
in src/pcl, as well as package-list.lisp.expr. The no-iterate patch
touches:
cache.lisp
construct.lisp
defcombin.lisp
methods.lisp
std-class.lisp
vector.lisp
in src/pcl. It was my guess that you would prefer patches with
narrowly-defined fixes (although a getting-rid-of-SB-ITERATE-totally
path might have been narrowly defined enough for you). I can do things
either way:
1) You apply the no-iterate patch and then I submit a no-gathering and
no-SB-ITERATE patch once I can diff from CVS, or
2) I generate a mega-patch that does everything at once.
Your call.
> a comment
>
> 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
[excellent explanation elided]
As this applies to the patch, would you prefer that I go back and clean
those comments up? I meant to do a little rewriting of them before
submitting the patch anyway (I think I cut-and-pasted one of the
comments in cache.lisp three or four times with oh-so-slight
variations).
--
Nathan | http://www.rose-hulman.edu/~froydnj/ | veritas aeterna
Yes, God had a deadline. So He wrote it all in Lisp.
Lisp. Everything else is just Turing complete.

On Wed, Dec 19, 2001 at 04:20:42PM -0500, Nathan Froyd wrote:
> On Wed, 2001-12-19 at 15:38, William Harold Newman wrote:
> > 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).
> > --
> > a question
> >
> > 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
> > single patch?
>
> The no-gathering patch touches:
>
> construct.lisp
> defclass.lisp
> defcombin.lisp
> dfun.lisp
> fast-init.lisp
> fngen.lisp
> macros.lisp
> std-class.lisp
> vector.lisp
>
> in src/pcl, as well as package-list.lisp.expr. The no-iterate patch
> touches:
>
> cache.lisp
> construct.lisp
> defcombin.lisp
> methods.lisp
> std-class.lisp
> vector.lisp
>
> in src/pcl. It was my guess that you would prefer patches with
> narrowly-defined fixes (although a getting-rid-of-SB-ITERATE-totally
> path might have been narrowly defined enough for you). I can do things
> either way:
>
> 1) You apply the no-iterate patch and then I submit a no-gathering and
> no-SB-ITERATE patch once I can diff from CVS, or
> 2) I generate a mega-patch that does everything at once.
>
> Your call.
OK, then why don't you do it as one big patch. (But I don't much care
either way. For me the usual reasons to break up patches are when they
can naturally be tested and/or rejected independently (e.g. a
conformance fix in PCL and an efficiency fix in X86 double-precision
floating point), or when they're hard to understand as a whole but
easier to understand as small comprehensible steps. Neither seems to
apply here.)
> As this applies to the patch, would you prefer that I go back and clean
> those comments up? I meant to do a little rewriting of them before
> submitting the patch anyway (I think I cut-and-pasted one of the
> comments in cache.lisp three or four times with oh-so-slight
> variations).
I would appreciate more careful comments on the weird bits. If you
don't add them yourself, I'll probably merge your patch anyway and try
to elaborate on the comments myself, but I'd appreciate it if you can
take care of it.
--
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