From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Patch to remove a bit of duplicated code in eval.c Date: Fri, 17 Sep 2021 10:29:35 +0300 Message-ID: <83k0jf8xsw.fsf@gnu.org> References: <87h7ekxkb1.fsf@gmail.com> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="35491"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Federico Tedin Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Fri Sep 17 09:31:20 2021 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mR8LA-00098V-KY for ged-emacs-devel@m.gmane-mx.org; Fri, 17 Sep 2021 09:31:20 +0200 Original-Received: from localhost ([::1]:49878 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mR8L9-00013e-Ax for ged-emacs-devel@m.gmane-mx.org; Fri, 17 Sep 2021 03:31:19 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:35008) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mR8Jn-0008Rf-9F for emacs-devel@gnu.org; Fri, 17 Sep 2021 03:29:56 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:52382) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mR8Jm-0003Lt-1Z; Fri, 17 Sep 2021 03:29:55 -0400 Original-Received: from 84.94.185.95.cable.012.net.il ([84.94.185.95]:4724 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mR8Jk-0002Nw-Vf; Fri, 17 Sep 2021 03:29:53 -0400 In-Reply-To: <87h7ekxkb1.fsf@gmail.com> (message from Federico Tedin on Thu, 16 Sep 2021 23:49:38 +0200) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:274843 Archived-At: > From: Federico Tedin > 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.