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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Alan Mackenzie
Hello, Emacs.

I've just had a bug report on bug-cc-mode from "Sun, Wei"
<[hidden email]> (Subject: bug#38691: CC Mode 5.34 (C++//lhw);
`Invalid search bound` when undo).

Essentially, this bug is reproduced by inserting a single line comment
(complete with CR) into an otherwise empty C++ Mode buffer:

    // 2019-12-20 17:57

, then doing
  C-x C-p (mark-page),
  C-u M-| <CR> sort <CR> (shell-command-on-region), then
  C-_ (undo).

This throws an error.

The cause of the error is that the C function call_process in
src/callproc.c is calling before-change-functions, but fails to call
after-change-functions.  This is at callproc.c ~L790, where there is the
helpful comment:

    /* FIXME: Call signal_after_change!  */

(thanks, Stefan!).

Well, I've tried adding this call to signal_after_change thusly:



diff --git a/src/callproc.c b/src/callproc.c
index b51594c2d5..6d74b34068 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -785,9 +785,11 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
     { /* We have to decode the input.  */
       Lisp_Object curbuf;
       ptrdiff_t count1 = SPECPDL_INDEX ();
+              ptrdiff_t beg;
 
       XSETBUFFER (curbuf, current_buffer);
       /* FIXME: Call signal_after_change!  */
+              beg = PT;
       prepare_to_modify_buffer (PT, PT, NULL);
       /* We cannot allow after-change-functions be run
  during decoding, because that might modify the
@@ -798,6 +800,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
       decode_coding_c_string (&process_coding,
       (unsigned char *) buf, nread, curbuf);
       unbind_to (count1, Qnil);
+              signal_after_change (beg, 0, PT - beg);
       if (display_on_the_fly
   && CODING_REQUIRE_DETECTION (&saved_coding)
   && ! CODING_REQUIRE_DETECTION (&process_coding))


, and this appears to solve the OP's problem.

However, a few lines further on, there's a del_range_2 call inside a condition
I don't understand (though might, with a great deal of study).  I suspect that
the call to signal_after_change ought to take this del_range_2 into account,
possibly coming after it.

Would somebody who's familiar with this bit of callproc.c please help me out
here, and explain what this call to del_range_2 is doing, and whether there's
anything basically wrong with my simple-minded addition of
signal_after_change.

Thanks!

--
Alan Mackenzie (Nuremberg, Germany).

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Date: Sat, 21 Dec 2019 17:23:24 +0000
> From: Alan Mackenzie <[hidden email]>
>
>        /* FIXME: Call signal_after_change!  */
> +              beg = PT;

Can you tell why you needed this variable and the assignment?  AFAIK,
PT doesn't change when we call decode_coding_c_string.

>        decode_coding_c_string (&process_coding,
>        (unsigned char *) buf, nread, curbuf);
>        unbind_to (count1, Qnil);
> +              signal_after_change (beg, 0, PT - beg);
>        if (display_on_the_fly
>    && CODING_REQUIRE_DETECTION (&saved_coding)
>    && ! CODING_REQUIRE_DETECTION (&process_coding))
>
>
> , and this appears to solve the OP's problem.
>
> However, a few lines further on, there's a del_range_2 call inside a condition
> I don't understand (though might, with a great deal of study).  I suspect that
> the call to signal_after_change ought to take this del_range_2 into account,
> possibly coming after it.
>
> Would somebody who's familiar with this bit of callproc.c please help me out
> here, and explain what this call to del_range_2 is doing, and whether there's
> anything basically wrong with my simple-minded addition of
> signal_after_change.

I'm not sure what you want to hear.  The del_range_2 call deletes the
just-inserted text, because the condition means that text was inserted
using the wrong coding-system to decode the incoming bytes.  What does
that mean for the modification hooks, I don't know: the
before-change-functions were already called, but nothing was inserted
from the Lisp application's POV, so if you insist on having before and
after hooks to be called in pairs, you are in a conundrum.

It's possible that we should simplify all this by calling the before
hooks just once before the loop and the after hooks just once after
the loop, instead of calling them for each individual chunk inside the
loop, but again I don't know what that means for applications which
expects these hook calls to pair.

Reply | Threaded
Open this post in threaded view
|

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

Alan Mackenzie
Hello, Eli.

Sorry my last post was not thought through.  I hope this one is better.

On Sat, Dec 21, 2019 at 20:11:01 +0200, Eli Zaretskii wrote:
> > Date: Sat, 21 Dec 2019 17:23:24 +0000
> > From: Alan Mackenzie <[hidden email]>
> >
> >        /* FIXME: Call signal_after_change!  */
> > +              beg = PT;

> Can you tell why you needed this variable and the assignment?  AFAIK,
> PT doesn't change when we call decode_coding_c_string.

I'd misunderstood decode_coding_c_string.  You're right, PT doesn't
change with this macro.  With this, the arguments I was getting to
after-change-functions were wrong.

However, I've kept BEG for the next version of the patch.

> >        decode_coding_c_string (&process_coding,
> >        (unsigned char *) buf, nread, curbuf);
> >        unbind_to (count1, Qnil);
> > +              signal_after_change (beg, 0, PT - beg);
> >        if (display_on_the_fly
> >    && CODING_REQUIRE_DETECTION (&saved_coding)
> >    && ! CODING_REQUIRE_DETECTION (&process_coding))
> >
> >
> > , and this appears to solve the OP's problem.
> >
> > However, a few lines further on, there's a del_range_2 call inside a condition
> > I don't understand (though might, with a great deal of study).  I suspect that
> > the call to signal_after_change ought to take this del_range_2 into account,
> > possibly coming after it.
> >
> > Would somebody who's familiar with this bit of callproc.c please help me out
> > here, and explain what this call to del_range_2 is doing, and whether there's
> > anything basically wrong with my simple-minded addition of
> > signal_after_change.

> I'm not sure what you want to hear.  The del_range_2 call deletes the
> just-inserted text, because the condition means that text was inserted
> using the wrong coding-system to decode the incoming bytes.  What does
> that mean for the modification hooks, I don't know: the
> before-change-functions were already called, but nothing was inserted
> from the Lisp application's POV, so if you insist on having before and
> after hooks to be called in pairs, you are in a conundrum.

I think I've solved this with the new variable prepared_position.  It
records the position of BEG for prepare_to_modify_buffer, and only calls
that function if it hasn't already done so for that BEG.  It's not an
elegant solution, but I think it will work.

> It's possible that we should simplify all this by calling the before
> hooks just once before the loop and the after hooks just once after
> the loop, instead of calling them for each individual chunk inside the
> loop, but again I don't know what that means for applications which
> expects these hook calls to pair.

I don't think this is necessary.  (If we positively want to do it,
that's a different matter.)

Here's the latest version of my patch.  It's only been slightly tested,
but it doesn't produce any unexpected "successes" from 'make check'.



diff --git a/src/callproc.c b/src/callproc.c
index b51594c2d5..34da7af863 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_position = 0;
+      ptrdiff_t beg;
 
       while (1)
  {
@@ -780,7 +782,13 @@ 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);
+            {
+              beg = PT;
+              insert_1_both (buf, nread, nread, 0, prepared_position < PT, 0);
+              if (prepared_position < PT)
+                prepared_position = PT;
+              signal_after_change (beg, 0, PT - beg);
+            }
   else
     { /* We have to decode the input.  */
       Lisp_Object curbuf;
@@ -788,7 +796,11 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 
       XSETBUFFER (curbuf, current_buffer);
       /* FIXME: Call signal_after_change!  */
-      prepare_to_modify_buffer (PT, PT, NULL);
+              if (prepared_position < PT)
+                {
+                  prepare_to_modify_buffer (PT, PT, NULL);
+                  prepared_position = PT;
+                }
       /* 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 +834,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;


--
Alan Mackenzie (Nuremberg, Germany).