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

[Alan Mackenzie]

Hello, Eli.

On Fri, Jan 03, 2020 at 10:45:53 +0200, Eli Zaretskii wrote:
> > Date: Sun, 29 Dec 2019 13:34:36 +0000
> > Cc: address@hidden
> > From: Alan Mackenzie <address@hidden>

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

It might, for example, cause some hook to invalidate some cache
unnecessarily.  But even if it caused no specific problem, it would cost
a needless runtime penalty.

[ .... ]

> > 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 I disagree with you generally on this point - I think it is
uneconomical to install a simple workaround when a complete fix is only a
little more difficult.  This change of mine is NOT complicated.   

[ .... ]

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

OK.  I have to say here, I really don't believe such an extensive
commentary is needed here.  The code is there, and anybody generally
familiar with our C code would understand it without a great deal of
difficulty, even the mechanism which prevents a spurious second call to
prepare_to_modify_buffer.  Surely?

Anyhow I've spent quite a bit of time trying to get the level of this
comment right, and my current version of it reads as follows:


          /* CHANGE FUNCTIONS
             For each iteration of the enclosing while (1) loop which
             yields data (i.e. nread > 0), before- and
             after-change-functions are each invoked exactly once.
             This is done directly from the current function only, by
             calling prepare_to_modify_buffer and signal_after_change.
             It is never done by directing another function such as
             insert_1_both to call them.  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, this
             function removing the newly inserted data.  At this stage
             prepare_to_modify_buffer has been called, but
             signal_after_change hasn't.  A continue statement
             restarts the enclosing while(1) loop.  A second,
             unwanted, call to `prepare_to_modify_buffer' is inhibited
             by the test prepared_pos < PT.  The data are inserted
             again, and this time signal_after_change gets called,
             balancing the previous call to prepare_to_modify_buffer.
          */

> Thanks (and apologies for a delay in responding).

That's OK, I haven't exactly been prompt myself in posting to this
thread.

-- 
Alan Mackenzie (Nuremberg, Germany).