# Common Lisp macro: review for code quality & leaky name escaping

(defmacro with-condition-retries(retries expected-errors fail-function &body body)
"Attempts to execute body retries times, watching for
expected-errors.  Optionally, if fail-function is set, each time a
failure occurs, prior to retrying, fail-function is executed.

Other conditions beside expected-errors will exit out of this macro"
(let ((counter (gensym))
(result (gensym))
(start-tag (gensym))
(error-handler
#'(lambda (c)
(let ((r (find-restart 'handler c)))
(when r
(invoke-restart r c))))))

(let ((,counter 0)
(,result))
(tagbody
,start-tag
(restart-case
(handler-bind
,(mapcar #'(lambda (error-val)
(list error-val   error-handler))
expected-errors)
(setf ,result ,@body))
;;the restart pointed at by the error-handler lambda
(handler (&rest args)
(declare (ignore args))
(incf ,counter)
(unless (> ,counter ,retries)
(when ,fail-function
(funcall ,fail-function))
(go ,start-tag)))))
,result)))


This is the sort of thing you want to get as many eyes on as possible. My gut tells me that you'd want to break it up into smaller functions, but I'm not entirely sure where I'd cut. Any chance of getting an example use or test case? Could you explain the reasoning behind using tagbody with a single go rather than a loop?

Preliminary stuff I've noticed

with-gensyms can save you a few lines at the beginning, and you should probably define it in a macro-heavy package.

(defmacro with-gensyms (syms &body body)
(let ,(loop for s in syms collect (,s (gensym)))
,@body))


The expression (setf ,result ,@body) might do some odd things if body is composed of multiple expressions. That should probably be (setf ,result (progn ,@body))` instead.

• I didn't want to scrape the loop facility to get exactly the right parameters - tagbody/go expresses things pretty well, if a rather imperatively. Also I've noticed loop in macros tends to require careful handling of quoting. – Paul Nathan Jan 31 '12 at 17:48