Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sun, 29 Dec 2019 13:34:36 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > 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).