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

[Eli Zaretskii <eliz@gnu.org>]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Thu, 16 Sep 2021 23:49:38 +0200
> 
> Reading eval.c I realized that there is very similar code in both
> 'eval_sub' and 'funcall_subr', where they invoke the subroutine itself.
> 
> I figured, since we have 'apply_lambda' (that gets called from
> 'eval_sub'), why not have an 'apply_subr' as well, to be used for
> subroutines? So I wrote a small patch (WIP) that adds 'apply_subr',
> which in turn calls 'funcall_subr'. I had to adapt 'funcall_subr' so
> that it accepts 'max_args=UNEVALLED' subroutines.
> 
> I think the advantages of doing this are that 1) it should make making
> changes to the structure of subroutines slightly easier (less code to
> update!) and 2) makes 'eval_sub' much more readable. In fact, now the
> function-calling part of 'eval_sub' is relatively short (~45 lines),
> which makes understanding the general structure of the function much
> easier in my opinion.

Thanks for working on this.

> 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 tried to compare the old and the new code, and their differences are
not trivial to audit.  For example, you remove this part:

> -      else if (XSUBR (fun)->max_args == UNEVALLED)
> -	val = (XSUBR (fun)->function.aUNEVALLED) (args_left);

but I don't immediately see how you have something equivalent and at
the right opportunity in the new code.

I'm not saying I already see something you have broken, I'm saying
these changes are not easy to audit for correctness, at least not for
me.  They are not a mechanical move of code from one place to another,
they really change the flow of control and processing.

So I think we'd like to have tests for each of the cases supported by
the code to make sure nothing is broken.  Is it feasible to write such
tests?

> 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.

If the reason for the comment is gone, no need to keep the comment.

> 3) Have I maybe made Emacs slower by always using SAFE_ALLOCA_LISP for
> the subroutine arguments (instead of only for 'max_args=MANY')?

This should be measured.

In any case, let's delay installing this patch until after the
emacs-28 release branch is cut, so as not to destabilize Emacs 28
unnecessarily.