Subject: Re: bug#36190: 27.0.50; `put-text-property' etc. with buffer argument calls current buffer's `after-change-functions'

From: Pip Cet <[email-masked]>
Date: Fri, 14 Jun 2019 11:14:53 +0000

On Fri, Jun 14, 2019 at 7:41 AM Eli Zaretskii <[email-masked]> wrote:
>
> > From: Pip Cet <[email-masked]>
> > Date: Thu, 13 Jun 2019 21:37:40 +0000
> > Cc: [email-masked]
> >
> > +/* Signal a change immediately after it happens.
> > +   BUFFER is the buffer in which the change happened.
> > +   CHARPOS is the character position of the start of the changed text.
> > +   LENDEL is the number of characters of the text before the change.
> > +   (Not the whole buffer; just the part that was changed.)
> > +   LENINS is the number of characters in that part of the text
> > +   after the change.  */
>
> I would just say "Like signal_after_change, but ..." and describe only
> the BUFFER argument.
>
> > +void
> > +signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
> > +{
> > +  ptrdiff_t count = SPECPDL_INDEX ();
> > +
> > +  record_unwind_current_buffer ();
> > +  set_buffer_internal (buffer);
> > +  signal_after_change (charpos, lendel, lenins);
> > +  unbind_to (count, Qnil);
> > +}
>
> I still think we should explicitly detect the current_buffer case here
> and if so, avoid the calls to everything else except
> signal_after_change itself.
>
> But I indeed like this patch better, although the concerns over the
> performance hit are still present.  Thanks.

Okay, I fixed those two issues. As for the performance problem, should
we amend the documentation to state that if many changes are made,
it's better to use `with-current-buffer' instead of repeatedly calling
put-text-property with a buffer argument?
From b23ebe451664b0f50a02fb2e4ee5a41877dfe32d Mon Sep 17 00:00:00 2001
From: Pip Cet <[email-masked]>
Date: Fri, 14 Jun 2019 11:05:31 +0000
Subject: [PATCH] Switch to correct buffer in put-text-property etc.

* src/textprop.c (add_text_properties_1, set_text_properties)
(Fremove_text_properties, Fremove_list_of_text_properties):
use `signal_after_change_in_buffer'.
* src/insdel.c (signal_after_change_in_buffer): New function.
---
 src/insdel.c   | 16 +++++++++++
 src/lisp.h     |  1 +
 src/textprop.c | 73 +++++++++++++++++++++++++++-----------------------
 3 files changed, 57 insertions(+), 33 deletions(-)

diff --git a/src/insdel.c b/src/insdel.c
index 85fffd8fd1..49aca94e03 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -2253,6 +2253,22 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
   unbind_to (count, Qnil);
 }
 
+/* Like `signal_after_change', but switch to BUFFER first.  */
+
+void
+signal_after_change_in_buffer (struct buffer *buffer, ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
+{
+  ptrdiff_t count = SPECPDL_INDEX ();
+
+  if (buffer != current_buffer)
+    {
+      record_unwind_current_buffer ();
+      set_buffer_internal (buffer);
+    }
+  signal_after_change (charpos, lendel, lenins);
+  unbind_to (count, Qnil);
+}
+
 static void
 Fcombine_after_change_execute_1 (Lisp_Object val)
 {
diff --git a/src/lisp.h b/src/lisp.h
index 77fc22d118..4e05c9555b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3686,6 +3686,7 @@ #define CONS_TO_INTEGER(cons, type, var)				\
 extern void prepare_to_modify_buffer_1 (ptrdiff_t, ptrdiff_t, ptrdiff_t *);
 extern void invalidate_buffer_caches (struct buffer *, ptrdiff_t, ptrdiff_t);
 extern void signal_after_change (ptrdiff_t, ptrdiff_t, ptrdiff_t);
+extern void signal_after_change_in_buffer (struct buffer *, ptrdiff_t, ptrdiff_t, ptrdiff_t);
 extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 				 ptrdiff_t, ptrdiff_t);
 extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..1781440b10 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1215,8 +1215,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  if (interval_has_all_properties (properties, i))
 	    {
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 
 	      eassert (modified);
 	      return Qt;
@@ -1226,8 +1226,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	    {
 	      add_properties (properties, i, object, set_type);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1237,8 +1237,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 	  copy_properties (unchanged, i);
 	  add_properties (properties, i, object, set_type);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				 XFIXNUM (end) - XFIXNUM (start));
+	    signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					   XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
 
@@ -1398,8 +1398,9 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
   set_text_properties_1 (start, end, properties, object, i);
 
   if (BUFFERP (object) && !NILP (coherent_change_p))
-    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-			 XFIXNUM (end) - XFIXNUM (start));
+    signal_after_change_in_buffer (XBUFFER (object),
+				   XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+				   XFIXNUM (end) - XFIXNUM (start));
   return Qt;
 }
 
@@ -1565,8 +1566,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      eassert (modified);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1574,8 +1576,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	    {
 	      remove_properties (properties, Qnil, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 
@@ -1585,8 +1588,9 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 	  copy_properties (unchanged, i);
 	  remove_properties (properties, Qnil, i, object);
 	  if (BUFFERP (object))
-	    signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				 XFIXNUM (end) - XFIXNUM (start));
+	    signal_after_change_in_buffer (XBUFFER (object),
+					   XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					   XFIXNUM (end) - XFIXNUM (start));
 	  return Qt;
 	}
 
@@ -1663,9 +1667,10 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 	      if (modified)
 		{
 		  if (BUFFERP (object))
-		    signal_after_change (XFIXNUM (start),
-					 XFIXNUM (end) - XFIXNUM (start),
-					 XFIXNUM (end) - XFIXNUM (start));
+		    signal_after_change_in_buffer (XBUFFER (object),
+						   XFIXNUM (start),
+						   XFIXNUM (end) - XFIXNUM (start),
+						   XFIXNUM (end) - XFIXNUM (start));
 		  return Qt;
 		}
 	      else
@@ -1677,8 +1682,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 	  else
@@ -1690,8 +1696,9 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 		modify_text_properties (object, start, end);
 	      remove_properties (Qnil, properties, i, object);
 	      if (BUFFERP (object))
-		signal_after_change (XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
-				     XFIXNUM (end) - XFIXNUM (start));
+		signal_after_change_in_buffer (XBUFFER (object),
+					       XFIXNUM (start), XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
 	      return Qt;
 	    }
 	}
@@ -1705,18 +1712,18 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
       len -= LENGTH (i);
       i = next_interval (i);
       if (!i)
-        {
-          if (modified)
-            {
-              if (BUFFERP (object))
-                signal_after_change (XFIXNUM (start),
-                                     XFIXNUM (end) - XFIXNUM (start),
-                                     XFIXNUM (end) - XFIXNUM (start));
-              return Qt;
-            }
-          else
-            return Qnil;
-        }
+	{
+	  if (modified)
+	    {
+	      if (BUFFERP (object))
+		signal_after_change_in_buffer (XBUFFER (object), XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start),
+					       XFIXNUM (end) - XFIXNUM (start));
+	      return Qt;
+	    }
+	  else
+	    return Qnil;
+	}
     }
 }
 
-- 
2.20.1