Re: [PATCH] Some improvements for cl-flet

[Stefan Monnier]

Hi,

Sorry for taking so long to review this.
This looks very complex for a macro that's used rather rarely.
Maybe it might be worthwhile splitting it into 2 or 3 patches so as to
better see how we got to the level of complexity.
See more comments below.

> --- a/lisp/emacs-lisp/cl-generic.el

I skipped this since `with-memoization` is now in `subr-x`.

> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
> index 6d6482c349..ecbe8e86fc 100644
> --- a/lisp/emacs-lisp/cl-macs.el
> +++ b/lisp/emacs-lisp/cl-macs.el
> @@ -2004,6 +2004,282 @@ defun cl--labels-convert (f)
>            (setq cl--labels-convert-cache (cons f res))
>            res))))))
>  
> +(defvar cl--flet-convert-with-setf-cache nil
> +  "Like `cl--labels-convert-cache' but for local setf functions.")
> +
> +(defun cl--flet-convert-with-setf (f)
> +  "Special macro-expander to rename (function F) references in `cl-flet', 
> including (function (setf F)).
> +
> +See also `cl--labels-convert'."
> +  ;; Note: If this function, or `cl--labels-convert', for that matter,
> +  ;; is redefined at runtime,
> +  ;; the whole replacement mechanism breaks!
> +  (if (and (consp f) (eq 'setf (car f)))
> +      (cond
> +       ;; We repeat lots of code from `cl--labels-convert'
> +       ((eq (cadr f) (car cl--flet-convert-with-setf-cache))
> +        (cdr cl--flet-convert-with-setf-cache))
> +       (t
> +        (let* ((found (assoc f macroexpand-all-environment #'equal))
> +               (replacement (and found
> +                                 (ignore-errors
> +                                   (funcall (cdr found) cl--labels-magic)))))
> +          (if (and replacement (eq cl--labels-magic (car replacement)))
> +              (nth 1 replacement)
> +            (let ((res `(function ,f)))
> +              (setq cl--flet-convert-with-setf-cache (cons (cadr f) res))
> +              res)))))
> +    (cl--labels-convert f)))

I didn't get to the point of trying to understand this.

> +(defmacro with--cl-flet-macroexp ( arglist var
> +                                   function-name expander memoized-alist
> +                                   &rest body)

All the defs should start with "cl-" so it should be `cl--with...`.

> +(defun cl--expand-local-setf (&rest places-and-values)
> +  "Expand `(setf . ,PLACES-AND-VALUES)
> +according to `cl--local-setf-expanders'.
> +
> +Presumes the caller has `macroexpand-all-environment' bound."

Why do we have/need this?  Does it work with other things that use
gv-places, like `push`, `pop`, `cl-callf`, ...?  If so, how?
If not, then we need another approach which does.

I thought handling `cl-flet` of (setf foo) would amount to calling
`gv-setter` to get the symbol corresponding to `(setf foo)` and then
c-flet-binding that symbol instead of `(setf foo).

> +(defun cl--expand-flet (env body &rest flet-expanders-plist)
> +  "Return a form equivalent to `(cl-flet ,bindings BODY)
> +where bindings correspond to FLET-EXPANDERS-PLIST as described below.
> +
> +ENV should be macroexpansion environment
> +to be augmented with some definitions from FLET-EXPANDERS-PLIST
> +to then expand forms in BODY with.
> +
> +FLET-EXPANDERS-PLIST should be a plist
> +where keys are function names
> +and values are 0-argument lambdas
> +to be called if the corresponding function name is encountered
> +in BODY and then only (that is, at most once).

Why "at most once"?

> +The return value of said lambdas should be either
> +
> +- a valid let-binding (SYMBOL function) to be used in let*
> +  bindings over BODY so that SYMBOL could be used in place of the
> +  corresponding function name in BODY
> +
> +or
> +
> +- a list (NIL EXPR) for EXPR to be used in BODY in place of the
> +  corresponding function name as is.

Can we simplify this so only one of the two is supported?


        Stefan