From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not? Date: Fri, 03 Jan 2020 10:45:53 +0200 Message-ID: <83eewhhqri.fsf@gnu.org> References: <20191221172324.GA8692@ACM> <83k16pzgzu.fsf@gnu.org> <20191221214751.GB8692@ACM> <83sglcxl1q.fsf@gnu.org> <20191224094724.GA3992@ACM> <837e2lwws9.fsf@gnu.org> <20191229133436.GA10699@ACM> Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="63494"; mail-complaints-to="usenet@blaine.gmane.org" Cc: emacs-devel@gnu.org To: Alan Mackenzie Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Jan 03 09:46:34 2020 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 1inIbJ-000GIw-MK for ged-emacs-devel@m.gmane.org; Fri, 03 Jan 2020 09:46:33 +0100 Original-Received: from localhost ([::1]:50040 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1inIbH-0004OJ-TQ for ged-emacs-devel@m.gmane.org; Fri, 03 Jan 2020 03:46:31 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:34848) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1inIaj-0003sg-4N for emacs-devel@gnu.org; Fri, 03 Jan 2020 03:45:58 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:43821) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1inIah-0006d6-97; Fri, 03 Jan 2020 03:45:55 -0500 Original-Received: from [176.228.60.248] (port=4841 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1inIag-0008M2-Kk; Fri, 03 Jan 2020 03:45:55 -0500 In-reply-to: <20191229133436.GA10699@ACM> (message from Alan Mackenzie on Sun, 29 Dec 2019 13:34:36 +0000) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] 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:243882 Archived-At: > Date: Sun, 29 Dec 2019 13:34:36 +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. I wasn't suggesting to produce unbalanced calls, I was suggesting that calling the hooks in a balanced way twice for the same position cannot be a serious problem. > > > 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. Really? I'd be surprised if such buggy hooks existed in any production code. How can a modification hook assume the insertion is always non-empty? > > 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. Sure, but the more complex the fix, the higher the risk. It's uneconomical to make fixes that are more complex than strictly necessary, I'm sure you agree. > > > 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? It falls short of what I'd like to see, because it doesn't cover the situation where this test: if (display_on_the_fly && CODING_REQUIRE_DETECTION (&saved_coding) && ! CODING_REQUIRE_DETECTION (&process_coding)) causes us to switch from using the 'else' branch to using the 'else if' branch in the following snippet: if (!nread) ; else if (NILP (BVAR (current_buffer, enable_multibyte_characters)) && ! CODING_MAY_REQUIRE_DECODING (&process_coding)) insert_1_both (buf, nread, nread, 0, 1, 0); else { /* We have to decode the input. */ IOW, the commentary you wrote doesn't tell the reader what insert_1_both, decode_coding_c_string, and del_range_2 do (or don't do) with regards to the modification hooks, and without that the comment is incomplete, and doesn't explain the logic of what the code does. Thanks (and apologies for a delay in responding).