[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Patch to remove a bit of duplicated code in eval.c
From: |
Stefan Monnier |
Subject: |
Re: Patch to remove a bit of duplicated code in eval.c |
Date: |
Fri, 17 Sep 2021 13:11:48 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
> @@ -3081,11 +2978,52 @@ DEFUN ("funcall", Ffuncall, Sfuncall, 1, MANY, 0,
> }
>
>
> +static Lisp_Object
> +apply_subr (struct Lisp_Subr *subr, Lisp_Object args, ptrdiff_t count)
> +{
I think this definition deserves a comment explaining at least what is
`count` (the other two are fairly self-explanatory, but not that one).
> + Lisp_Object *arg_vector;
> + Lisp_Object tem;
> + USE_SAFE_ALLOCA;
> +
> + ptrdiff_t numargs = list_length (args);
> +
> + if (subr->max_args != UNEVALLED)
> + {
> + Lisp_Object args_left = args;
> + SAFE_ALLOCA_LISP (arg_vector, numargs);
> +
> + for (ptrdiff_t i = 0; i < numargs; i++)
> + {
> + tem = Fcar (args_left);
> + args_left = Fcdr(args_left);
> + tem = eval_sub(tem);
[ Be careful to remember to put a space before the open parens. ]
> Lisp_Object
> -funcall_subr (struct Lisp_Subr *subr, ptrdiff_t numargs, Lisp_Object *args)
> +funcall_subr (struct Lisp_Subr *subr, ptrdiff_t numargs, Lisp_Object *args,
> bool unevalled_ok)
> {
I'm not very happy with this.
Everywhere else in Emacs, the name "funcall" means we're calling
a *function* and not a special form. I think we'd be better off keeping
`funcall_subr` unchanged and use "something else" when `+apply_subr`
needs to handle a special form (aka `UNEVALLED`).
That will also make it obvious that the patch does not slow down
execution of bytecode at all (which does use `funcall_subr` but
not `eval_sub`).
> My concerns now are:
> 1) Could I have broken anything without realizing it, since this is such
> a central function in Lisp code evaluation? Everything seems to be
> compiling fine (without warnings) and so far I haven't had any crashes.
I haven't looked in enough details to be sure, but in principle it
should be OK since it re-uses the well-tested `funcall_subr` code.
> 2) I removed a comment that made reference to Bug#21245, but it seems
> like it makes sense since the variable it refers to is no longer needed.
That removal looks good, thanks.
> 3) Have I maybe made Emacs slower by always using SAFE_ALLOCA_LISP for
> the subroutine arguments (instead of only for 'max_args=MANY')?
It might slightly slow down execution of interpreted code, but
interpreted code should not be performance critical (after all, if
speed matters, the answer is to byte-compile the code). You can try and
measure the slowdown in the following way:
rm src/*.pdmp lisp/**/*.elc
(cd src; make bootstrap-emacs.pdmp)
rm lisp/**/*.elc
(cd lisp; time make emacs-lisp/macroexp.elc)
The important part is to time the `make emacs-lisp/macroexp.elc`.
The three lines before it only serve to get to a state where we have
a working Emacs executable with no bytecode at all (so the compilation
of `macroexp.el` takes a long while because all the code is
interpreted).
Stefan