Subject: bug#77924: 31.0.50; [Feature branch] Change marker implementation

From: Pip Cet <[email-masked]>
Date: Thu, 24 Apr 2025 19:26:10 +0000

Gerd Möllmann <[email-masked]> writes:

> Eli Zaretskii <[email-masked]> writes:
>
>>> +  DO_MARKERS (b, m)
>>> +    {
>>> +      if (!vectorlike_marked_p (&m->header))
>>> +	marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
>>> +    }
>>> +  END_DO_MARKERS;
>>
>> Would it be possible not to introduce macros that modify the C syntax?
>> These are problematic for more than one reason (one being that they
>> are not recognized by C modes we use).  We have macros whose names
>> start with FOR_EACH_ (like FOR_EACH_FRAME); can we introduce
>> FOR_EACH_MARKER instead, and not hide opening/closing braces inside
>> macros?
>
> I tried, already in igc, but failed. IOW, I don't now a way to get rid
> of the END_DO_MARKERS.

Okay, I'll bite.

I think what we should do is mimic FOR_EACH_TAIL, and use
FOR_EACH_MARKER like this:

struct Lisp_Marker *m;
FOR_EACH_MARKER (b, m)
  {
    /* do something with m */
  }

However, if we wanted to, we could do this (the "continued" thing is
overkill to make "break" work in a FOR_EACH_MARKER loop):

diff --git a/src/marker-vector.h b/src/marker-vector.h
index 141e79ac0ee..bfca02fc33d 100644
--- a/src/marker-vector.h
+++ b/src/marker-vector.h
@@ -39,27 +39,55 @@ #define EMACS_MARKER_VECTOR_H
   MARKER_VECTOR_ENTRY_SIZE = 2,
 };
 
-/* Iterate over markers in marker vector MV, binding a variable with
-   name M to a pointer to Lisp_Marker.  The loop must be ended
-   with an END_DO_MARKERS.  */
-
-# define DO_MARKERS_OF_VECTOR(mv, m)					\
-  for (ptrdiff_t e_ = MARKER_VECTOR_HEADER_SIZE,			\
-	 end_ = XFIXNUM (AREF (mv, MARKER_VECTOR_MAX_ENTRY));		\
-       e_ <= end_;							\
-       e_ += MARKER_VECTOR_ENTRY_SIZE)					\
-    {									\
-       Lisp_Object m_ = AREF (mv, e_ + MARKER_VECTOR_OFFSET_MARKER);	\
-       if (MARKERP (m_))						\
-	 {								\
-            struct Lisp_Marker *m = XMARKER (m_);
-
-/* Iterate over markers of buffer B, binding a variable with name M to a
-   pointer to Lisp_Marker.  The loop must be ended with an
-   END_DO_MARKERS.  */
-
-# define DO_MARKERS(b, m) DO_MARKERS_OF_VECTOR (BUF_MARKERS (b), m)
-# define END_DO_MARKERS }}
+struct for_each_marker_data
+{
+  ptrdiff_t e;
+  ptrdiff_t end;
+  Lisp_Object m;
+  Lisp_Object mv;
+  bool continued;
+  struct Lisp_Marker *marker;
+};
+
+INLINE struct for_each_marker_data
+build_for_each_marker_data(Lisp_Object mv)
+{
+  struct for_each_marker_data ret;
+  ret.e = MARKER_VECTOR_HEADER_SIZE;
+  ret.end = XFIXNUM (AREF (mv, MARKER_VECTOR_MAX_ENTRY));
+  ret.m = Qnil;
+  ret.mv = mv;
+  ret.marker = NULL;
+  ret.continued = true;
+  return ret;
+}
+
+INLINE bool
+next_marker_entry (struct for_each_marker_data *d)
+{
+  if (!d->continued)
+    return false;
+  while (d->e <= d->end)
+    {
+      d->m = AREF (d->mv, d->e + MARKER_VECTOR_OFFSET_MARKER);
+      d->e += MARKER_VECTOR_ENTRY_SIZE;
+      if (MARKERP (d->m))
+	{
+	  d->marker = XMARKER (d->m);
+	  d->continued = false;
+	  return true;
+	}
+    }
+  return false;
+}
+
+#define FOR_EACH_MARKER_OF_VECTOR(v, m)					\
+  for (struct for_each_marker_data d_ = build_for_each_marker_data (v); \
+       next_marker_entry (&d_);)					\
+    for (struct Lisp_Marker *m = d_.marker; !d_.continued; d_.continued = true)
+
+#define FOR_EACH_MARKER(b, m) \
+  FOR_EACH_MARKER_OF_VECTOR(BUF_MARKERS (b), m)
 
 Lisp_Object make_marker_vector (void);
 Lisp_Object alloc_marker_vector (ptrdiff_t len);
diff --git a/src/alloc.c b/src/alloc.c
index c90f60ee518..30bdd1140b8 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -7114,12 +7114,11 @@ sweep_symbols (void)
 static void
 unchain_dead_markers (struct buffer *b)
 {
-  DO_MARKERS (b, m)
+  FOR_EACH_MARKER (b, m)
     {
       if (!vectorlike_marked_p (&m->header))
 	marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
     }
-  END_DO_MARKERS;
 }
 
 NO_INLINE /* For better stack traces */
diff --git a/src/buffer.c b/src/buffer.c
index 93a2edb3693..21b6096517d 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -2073,12 +2073,11 @@ DEFUN ("kill-buffer", Fkill_buffer, Skill_buffer, 0, 1, "bKill buffer: ",
       /* Unchain all markers that belong to this indirect buffer.
 	 Don't unchain the markers that belong to the base buffer
 	 or its other indirect buffers.  */
-      DO_MARKERS (b, m)
+      FOR_EACH_MARKER (b, m)
 	{
 	  if (m->buffer == b)
 	    marker_vector_remove (XVECTOR (BUF_MARKERS (b)), m);
 	}
-      END_DO_MARKERS;
 
      /* Intervals should be owned by the base buffer (Bug#16502).  */
       i = buffer_intervals (b);
@@ -2618,7 +2617,7 @@ #define swapfield_(field, type) \
   other_buffer->text->end_unchanged = other_buffer->text->gpt;
   swap_buffer_overlays (current_buffer, other_buffer);
   {
-    DO_MARKERS (current_buffer, m)
+    FOR_EACH_MARKER (current_buffer, m)
       {
 	if (m->buffer == other_buffer)
 	  m->buffer = current_buffer;
@@ -2627,8 +2626,7 @@ #define swapfield_(field, type) \
 	     BUF_MARKERS(buf) should either be for `buf' or dead.  */
 	  eassert (!m->buffer);
       }
-    END_DO_MARKERS;
-    DO_MARKERS (other_buffer, m)
+    FOR_EACH_MARKER (other_buffer, m)
       {
 	if (m->buffer == current_buffer)
 	  m->buffer = other_buffer;
@@ -2637,7 +2635,6 @@ #define swapfield_(field, type) \
 	     BUF_MARKERS(buf) should either be for `buf' or dead.  */
 	  eassert (!m->buffer);
       }
-    END_DO_MARKERS;
   }
 
   { /* Some of the C code expects that both window markers of a
@@ -2746,12 +2743,11 @@ DEFUN ("set-buffer-multibyte", Fset_buffer_multibyte, Sset_buffer_multibyte,
       GPT = GPT_BYTE;
       TEMP_SET_PT_BOTH (PT_BYTE, PT_BYTE);
 
-      DO_MARKERS (current_buffer, tail)
+      FOR_EACH_MARKER (current_buffer, tail)
 	{
 	  const ptrdiff_t bytepos = marker_vector_bytepos (tail);
 	  marker_vector_set_charpos (tail, bytepos);
 	}
-      END_DO_MARKERS;
 
       /* Convert multibyte form of 8-bit characters to unibyte.  */
       pos = BEG;
@@ -2901,13 +2897,12 @@ DEFUN ("set-buffer-multibyte", Fset_buffer_multibyte, Sset_buffer_multibyte,
 	TEMP_SET_PT_BOTH (position, byte);
       }
 
-      DO_MARKERS (current_buffer, tail)
+      FOR_EACH_MARKER (current_buffer, tail)
 	{
 	  ptrdiff_t bytepos = marker_vector_bytepos (tail);
 	  bytepos = advance_to_char_boundary (bytepos);
 	  marker_vector_set_charpos (tail, BYTE_TO_CHAR (bytepos));
 	}
-      END_DO_MARKERS;
 
       /* Do this last, so it can calculate the new correspondences
 	 between chars and bytes.  */
diff --git a/src/coding.c b/src/coding.c
index f3dd2639e73..51c87fb3161 100644
--- a/src/coding.c
+++ b/src/coding.c
@@ -8118,14 +8118,13 @@ decode_coding_object (struct coding_system *coding,
 	move_gap_both (from, from_byte);
       if (BASE_EQ (src_object, dst_object))
 	{
-	  DO_MARKERS (current_buffer, tail)
+	  FOR_EACH_MARKER (current_buffer, tail)
 	    {
 	      const ptrdiff_t charpos = marker_vector_charpos (tail);
 	      tail->need_adjustment
 		= charpos == (tail->insertion_type ? from : to);
 	      need_marker_adjustment |= tail->need_adjustment;
 	    }
-	  END_DO_MARKERS;
 	  saved_pt = PT, saved_pt_byte = PT_BYTE;
 	  TEMP_SET_PT_BOTH (from, from_byte);
 	  current_buffer->text->inhibit_shrinking = true;
@@ -8250,7 +8249,7 @@ decode_coding_object (struct coding_system *coding,
 
       if (need_marker_adjustment)
 	{
-	  DO_MARKERS (current_buffer, tail)
+	  FOR_EACH_MARKER (current_buffer, tail)
 	    {
 	      if (tail->need_adjustment)
 		{
@@ -8269,7 +8268,6 @@ decode_coding_object (struct coding_system *coding,
 		    }
 		}
 	    }
-	  END_DO_MARKERS;
 	}
     }
 
@@ -8340,14 +8338,13 @@ encode_coding_object (struct coding_system *coding,
   if (BASE_EQ (src_object, dst_object) && BUFFERP (src_object))
     {
       same_buffer = true;
-      DO_MARKERS (XBUFFER (src_object), tail)
+      FOR_EACH_MARKER (XBUFFER (src_object), tail)
 	{
 	  const ptrdiff_t charpos = marker_vector_charpos (tail);
 	  tail->need_adjustment
 	    = charpos == (tail->insertion_type ? from : to);
 	  need_marker_adjustment |= tail->need_adjustment;
 	}
-      END_DO_MARKERS;
     }
 
   if (! NILP (CODING_ATTR_PRE_WRITE (attrs)))
@@ -8505,7 +8502,7 @@ encode_coding_object (struct coding_system *coding,
 
       if (need_marker_adjustment)
 	{
-	  DO_MARKERS (current_buffer, tail)
+	  FOR_EACH_MARKER (current_buffer, tail)
 	    {
 	      if (tail->need_adjustment)
 		{
@@ -8524,7 +8521,6 @@ encode_coding_object (struct coding_system *coding,
 		    }
 		}
 	    }
-	  END_DO_MARKERS;
 	}
     }
 
diff --git a/src/editfns.c b/src/editfns.c
index 61cb7bdd201..b96275dc0a3 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -4438,7 +4438,7 @@ transpose_markers (ptrdiff_t start1, ptrdiff_t end1,
   amt1 = (end2 - start2) + (start2 - end1);
   amt2 = (end1 - start1) + (start2 - end1);
 
-  DO_MARKERS (current_buffer, marker)
+  FOR_EACH_MARKER (current_buffer, marker)
     {
       mpos = marker_vector_charpos (marker);
       if (mpos >= start1 && mpos < end2)
@@ -4452,7 +4452,6 @@ transpose_markers (ptrdiff_t start1, ptrdiff_t end1,
 	}
       marker_vector_set_charpos (marker, mpos);
     }
-  END_DO_MARKERS;
 }
 
 DEFUN ("transpose-regions", Ftranspose_regions, Stranspose_regions, 4, 5,
diff --git a/src/marker-vector.c b/src/marker-vector.c
index b65bbe15845..cd2603f9561 100644
--- a/src/marker-vector.c
+++ b/src/marker-vector.c
@@ -164,7 +164,7 @@ check_marker_vector (struct Lisp_Vector *v, bool allocating)
 
   size_t nused = 0;
   Lisp_Object mv = make_lisp_ptr (v, Lisp_Vectorlike);
-  DO_MARKERS_OF_VECTOR (mv, m)
+  FOR_EACH_MARKER_OF_VECTOR (mv, m)
     {
       eassert (m->entry == e_);
       eassert (m->buffer != NULL);
@@ -175,7 +175,6 @@ check_marker_vector (struct Lisp_Vector *v, bool allocating)
 	}
       ++nused;
     }
-  END_DO_MARKERS;
 
   eassert ((nused + nfree) * MARKER_VECTOR_ENTRY_SIZE
 	   + MARKER_VECTOR_HEADER_SIZE == gc_vsize (v));
@@ -286,11 +285,10 @@ marker_vector_reset (struct buffer *b)
   /* The old GC contains at least one assertion that unchaining markers
      in kill-buffer resets the markers' buffers.  IGC does not do this,
      can't do this, and does not need it.  */
-  DO_MARKERS (b, m)
+  FOR_EACH_MARKER (b, m)
     {
       m->buffer = NULL;
     }
-  END_DO_MARKERS;
   BUF_MARKERS (b) = Qnil;
 }
 
@@ -348,7 +346,7 @@ marker_vector_adjust_for_insert (struct buffer *b,
 {
   const ptrdiff_t nchars = to_charpos - from_charpos;
   struct Lisp_Vector *v = XVECTOR (BUF_MARKERS (b));
-  DO_MARKERS (b, m)
+  FOR_EACH_MARKER (b, m)
     {
       const ptrdiff_t charpos = XFIXNUM (CHARPOS (v, m->entry));
       if (charpos == from_charpos)
@@ -359,7 +357,6 @@ marker_vector_adjust_for_insert (struct buffer *b,
       else if (charpos > from_charpos)
 	CHARPOS (v, m->entry) = make_fixnum (charpos + nchars);
     }
-  END_DO_MARKERS;
 }
 
 /* Adjust marker positions of buffer Bs for a replacement of text at
@@ -375,7 +372,7 @@ marker_vector_adjust_for_replace (struct buffer *b,
   const ptrdiff_t diff_nchars = new_nchars - old_nchars;
   const ptrdiff_t old_to_charpos = from_charpos + old_nchars;
   struct Lisp_Vector *v = XVECTOR (BUF_MARKERS (b));
-  DO_MARKERS (b, m)
+  FOR_EACH_MARKER (b, m)
     {
       const ptrdiff_t charpos = XFIXNUM (CHARPOS (v, m->entry));
       if (charpos >= old_to_charpos)
@@ -383,5 +380,4 @@ marker_vector_adjust_for_replace (struct buffer *b,
       else if (charpos > from_charpos)
 	CHARPOS (v, m->entry) = make_fixnum (from_charpos);
     }
-  END_DO_MARKERS;
 }
diff --git a/src/undo.c b/src/undo.c
index 65de0bd4e13..3e63c8af3f9 100644
--- a/src/undo.c
+++ b/src/undo.c
@@ -128,7 +128,7 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
 {
   prepare_record ();
 
-  DO_MARKERS (current_buffer, m)
+  FOR_EACH_MARKER (current_buffer, m)
     {
       ptrdiff_t charpos = marker_vector_charpos (m);
       eassert (charpos <= Z);
@@ -154,7 +154,6 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to)
             }
         }
     }
-  END_DO_MARKERS;
 }
 
 /* Record that a deletion is about to take place, of the characters in

Note that "m" appears only once, so there is another possibility:

FOR_EACH_MARKER (v, struct Lisp_Marker *m)
  {
    /* do something with m */
  }

I'm not saying any of this is a good idea, but it appears possible.

Pip