From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.devel Subject: Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not? Date: Sun, 29 Dec 2019 13:34:36 +0000 Message-ID: <20191229133436.GA10699@ACM> References: <20191221172324.GA8692@ACM> <83k16pzgzu.fsf@gnu.org> <20191221214751.GB8692@ACM> <83sglcxl1q.fsf@gnu.org> <20191224094724.GA3992@ACM> <837e2lwws9.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="6413"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.10.1 (2018-07-13) Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Dec 29 14:35:27 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1ilYj9-0001WO-HA for ged-emacs-devel@m.gmane.org; Sun, 29 Dec 2019 14:35:27 +0100 Original-Received: from localhost ([::1]:52190 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ilYj7-0003Hp-8U for ged-emacs-devel@m.gmane.org; Sun, 29 Dec 2019 08:35:25 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:58092) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ilYiv-0003Hi-FM for emacs-devel@gnu.org; Sun, 29 Dec 2019 08:35:17 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ilYit-0001bh-Lv for emacs-devel@gnu.org; Sun, 29 Dec 2019 08:35:13 -0500 Original-Received: from colin.muc.de ([193.149.48.1]:27512 helo=mail.muc.de) by eggs.gnu.org with smtp (Exim 4.71) (envelope-from ) id 1ilYiO-0008NJ-7Y for emacs-devel@gnu.org; Sun, 29 Dec 2019 08:35:11 -0500 Original-Received: (qmail 78147 invoked by uid 3782); 29 Dec 2019 13:34:38 -0000 Original-Received: from acm.muc.de (p2E5D51F6.dip0.t-ipconnect.de [46.93.81.246]) by colin.muc.de (tmda-ofmipd) with ESMTP; Sun, 29 Dec 2019 14:34:36 +0100 Original-Received: (qmail 11334 invoked by uid 1000); 29 Dec 2019 13:34:36 -0000 Content-Disposition: inline In-Reply-To: <837e2lwws9.fsf@gnu.org> X-Delivery-Agent: TMDA/1.1.12 (Macallan) X-Primary-Address: acm@muc.de X-detected-operating-system: by eggs.gnu.org: FreeBSD 9.x [fuzzy] X-Received-From: 193.149.48.1 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.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:243745 Archived-At: Hello, Eli. I've combined your two recent posts into one, to make it easier to answer them. On Tue, Dec 24, 2019 at 17:47:18 +0200, Eli Zaretskii wrote: > > Date: Tue, 24 Dec 2019 09:47:24 +0000 > > Cc: emacs-devel@gnu.org > > From: Alan Mackenzie > > The point is not to call prepare_to_modify_buffer twice at the same > > position. > Why is that a problem? Surely, something like that can happen in real > life, and any modification hook should be prepared to deal with that? Well, the more we can issue balanced before- and after- calls, the better. It is true that the hook functions should be able to handle unbalanced calls, but we don't know for sure that they can, in all cases. CC Mode might not be the only casualty (though I intend to fix CC Mode, here). Besides, the Elisp manual page "Change Hooks" only describes one situation for unbalanced calls. This is one large enclosing before- followed by a sequence of smaller after-s. The situation in callproc.c is thus technically a bug. > > > I think you should simply call signal_after_change after the call to > > > del_range_2 (telling the after-change hooks that actually nothing was > > > inserted or deleted). Then you won't need the prepared_position > > > thingy. > > After thinking it over a couple of days, I can't agree this is a good > > idea. Calling before/after-change-functions for a non-change would be > > very unusual in Emacs - I don't know of anywhere where this is currently > > done - and would surely cause problems somewhere, and would certainly > > cause some inefficiency. Also we would have to amend the Change Hooks > > page in the Elisp manual to warn of this possibility. > Again, I don't see why this could cause any trouble. Inserting an > empty string is not an outlandish situation, and any modification hook > must be prepared to (trivially) deal with it. This may be true, but I wouldn't bet anything on it being true for all existing hooks. Doing this would necessitate amending the Elisp manual, and (if we're going to be honest) adding a NEWS item in the incompatible changes section. > IOW, jumping through hoops in order to avoid such calls is IMNSHO > unjustified. It will definitely complicate code, and thus will run > higher risk of subtle bugs. Why risk that? We have a real existing bug here, and any fix to it runs the risk of further bugs. I think introducing no-change change hooks is more likely to cause trouble than the relatively straightforward patch I'm proposing. My proposed patch is not complicated. Really, it isn't. It's the least messy way of fixing the bug we have. > > I think the following patch is better. What do you think? > Frankly, I don't like this, for the reasons I explained in my other > message. If you insist on jumping through these hoops just to avoid > an extra call to the modification hooks, please write a comment with > the detailed description of the logic of these calls and their > conditions, and how that ensures the paired calls with no extra calls. OK, I've done this. Writing this comment was actually more difficult than amending the code. :-) The current state of my proposed patch, unchanged except for the new comment, follows. Comments? diff --git a/src/callproc.c b/src/callproc.c index b51594c2d5..66eebdebb4 100644 --- a/src/callproc.c +++ b/src/callproc.c @@ -746,6 +746,8 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd, int carryover = 0; bool display_on_the_fly = display_p; struct coding_system saved_coding = process_coding; + ptrdiff_t prepared_pos = 0; /* prepare_to_modify_buffer was last + called here. */ while (1) { @@ -773,6 +775,27 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd, if (display_on_the_fly) break; } + /* CHANGE FUNCTIONS + For each typical iteration of the enclosing while (1) + loop which yields data (i.e. nread > 0), before- and + after-change-functions are invoked exactly once. The + call to prepare_to_modify_buffer follows this comment, + and there is one call to signal_after_change in each of + the branches of the next `else if'. + + Exceptionally, the insertion into the buffer is aborted + at the call to del_range_2 ~45 lines further down, + without signal_after_change being called. The data are + then inserted finally at the next iteration of the + while (1) loop. A second, unwanted, call to + prepare_to_modify_buffer is inhibited by the test + prepared_pos < PT, and signal_after_change is then + called normally after the insertion. */ + if ((prepared_pos < PT) && nread) + { + prepare_to_modify_buffer (PT, PT, NULL); + prepared_pos = PT; + } /* Now NREAD is the total amount of data in the buffer. */ @@ -780,15 +803,16 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd, ; else if (NILP (BVAR (current_buffer, enable_multibyte_characters)) && ! CODING_MAY_REQUIRE_DECODING (&process_coding)) - insert_1_both (buf, nread, nread, 0, 1, 0); + { + insert_1_both (buf, nread, nread, 0, 0, 0); + signal_after_change (PT, 0, nread); + } else { /* We have to decode the input. */ Lisp_Object curbuf; ptrdiff_t count1 = SPECPDL_INDEX (); XSETBUFFER (curbuf, current_buffer); - /* FIXME: Call signal_after_change! */ - prepare_to_modify_buffer (PT, PT, NULL); /* We cannot allow after-change-functions be run during decoding, because that might modify the buffer, while we rely on process_coding.produced to @@ -822,6 +846,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd, continue; } + signal_after_change (PT, 0, process_coding.produced_char); TEMP_SET_PT_BOTH (PT + process_coding.produced_char, PT_BYTE + process_coding.produced); carryover = process_coding.carryover_bytes; > Thanks. -- Alan Mackenzie (Nuremberg, Germany).