Re: Patch to remove a bit of duplicated code in eval.c

[Stefan Monnier <monnier@iro.umontreal.ca>]

> @@ -3081,11 +2978,52 @@ DEFUN ("funcall", Ffuncall, Sfuncall, 1, MANY, 0,
>  }
>  \f
>  
> +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