Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109512: Inline functions to examine and change buffer overlays.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109512: Inline functions to examine and change buffer overlays.

[Stefan Monnier]

Can we pause for a moment?
As I mentioned to you earlier, you should post such patches for review
before installing them.
And explain why you think they're good.
E.g. replacing buffer->overlays_before by "buffer_get_overlays (buffer,
OV_BEFORE)" is far from "obviously good" in my eyes.
If you intend to make those field accesses into something more complex,
then please say so and explain why.  If not, then please stick to
"foo->bar", as agreed for the FGET case.


        Stefan


>>>>> "Dmitry" == Dmitry Antipov <dmantipov@yandex.ru> writes:

> ------------------------------------------------------------
> revno: 109512
> committer: Dmitry Antipov <dmantipov@yandex.ru>
> branch nick: trunk
> timestamp: Wed 2012-08-08 18:47:11 +0400
> message:
>   Inline functions to examine and change buffer overlays.
>   * buffer.c (unchain_both): New function.
>   * buffer.h (buffer_get_overlays, buffer_set_overlays):
>   (buffer_has_overlays): New function.
>   (enum overlay_type): New enum.
>   * alloc.c, buffer.c, editfns.c, fileio.c, indent.c:
>   * insdel.c, intervals.c, print.c, xdisp.c: Adjust users.
> modified:
>   src/ChangeLog
>   src/alloc.c
>   src/buffer.c
>   src/buffer.h
>   src/editfns.c
>   src/fileio.c
>   src/indent.c
>   src/insdel.c
>   src/intervals.c
>   src/print.c
>   src/xdisp.c

> === modified file 'src/ChangeLog'
> --- a/src/ChangeLog	2012-08-08 12:12:40 +0000
> +++ b/src/ChangeLog	2012-08-08 14:47:11 +0000
> @@ -1,5 +1,15 @@
>  2012-08-08  Dmitry Antipov  <dmantipov@yandex.ru>
 
> +	Inline functions to examine and change buffer overlays.
> +	* buffer.c (unchain_both): New function.
> +	* buffer.h (buffer_get_overlays, buffer_set_overlays):
> +	(buffer_has_overlays): New function.
> +	(enum overlay_type): New enum.
> +	* alloc.c, buffer.c, editfns.c, fileio.c, indent.c:
> +	* insdel.c, intervals.c, print.c, xdisp.c: Adjust users.
> +
> +2012-08-08  Dmitry Antipov  <dmantipov@yandex.ru>
> +
>  	Inline functions to examine and change buffer intervals.
>  	* alloc.c (mark_interval_tree): Remove.
>  	(MARK_INTERVAL_TREE): Simplify.

> === modified file 'src/alloc.c'
> --- a/src/alloc.c	2012-08-08 12:12:40 +0000
> +++ b/src/alloc.c	2012-08-08 14:47:11 +0000
> @@ -5831,8 +5831,8 @@
>       a special way just before the sweep phase, and after stripping
>       some of its elements that are not needed any more.  */
 
> -  mark_overlay (buffer->overlays_before);
> -  mark_overlay (buffer->overlays_after);
> +  mark_overlay (buffer_get_overlays (buffer, OV_BEFORE));
> +  mark_overlay (buffer_get_overlays (buffer, OV_AFTER));
 
>    /* If this is an indirect buffer, mark its base buffer.  */
>    if (buffer->base_buffer && !VECTOR_MARKED_P (buffer->base_buffer))

> === modified file 'src/buffer.c'
> --- a/src/buffer.c	2012-08-08 12:12:40 +0000
> +++ b/src/buffer.c	2012-08-08 14:47:11 +0000
> @@ -474,8 +474,10 @@
 
>    memcpy (to->local_flags, from->local_flags, sizeof to->local_flags);
 
> -  to->overlays_before = copy_overlays (to, from->overlays_before);
> -  to->overlays_after = copy_overlays (to, from->overlays_after);
> +  buffer_set_overlays
> +    (to, copy_overlays (to, buffer_get_overlays (from, OV_BEFORE)), OV_BEFORE);
> +  buffer_set_overlays
> +    (to, copy_overlays (to, buffer_get_overlays (from, OV_AFTER)), OV_AFTER);
 
>    /* Get (a copy of) the alist of Lisp-level local variables of FROM
>       and install that in TO.  */
> @@ -674,21 +676,22 @@
>  {
>    struct Lisp_Overlay *ov, *next;
 
> -  for (ov = b->overlays_before; ov; ov = next)
> -    {
> -      drop_overlay (b, ov);
> -      next = ov->next;
> -      ov->next = NULL;
> -    }
> -
> -  for (ov = b->overlays_after; ov; ov = next)
> -    {
> -      drop_overlay (b, ov);
> -      next = ov->next;
> -      ov->next = NULL;
> -    }
> -
> -  b->overlays_before = b->overlays_after = NULL;
> +  for (ov = buffer_get_overlays (b, OV_BEFORE); ov; ov = next)
> +    {
> +      drop_overlay (b, ov);
> +      next = ov->next;
> +      ov->next = NULL;
> +    }
> +
> +  for (ov = buffer_get_overlays (b, OV_AFTER); ov; ov = next)
> +    {
> +      drop_overlay (b, ov);
> +      next = ov->next;
> +      ov->next = NULL;
> +    }
> +
> +  buffer_set_overlays (b, NULL, OV_BEFORE);
> +  buffer_set_overlays (b, NULL, OV_AFTER);
>  }
 
>  /* Reinitialize everything about a buffer except its name and contents
> @@ -716,8 +719,8 @@
b-> auto_save_failure_time = 0;
>    BVAR (b, auto_save_file_name) = Qnil;
>    BVAR (b, read_only) = Qnil;
> -  b->overlays_before = NULL;
> -  b->overlays_after = NULL;
> +  buffer_set_overlays (b, NULL, OV_BEFORE);
> +  buffer_set_overlays (b, NULL, OV_AFTER);
b-> overlay_center = BEG;
>    BVAR (b, mark_active) = Qnil;
>    BVAR (b, point_before_scroll) = Qnil;
> @@ -2602,7 +2605,7 @@
>    ptrdiff_t prev = BEGV;
>    int inhibit_storing = 0;
 
> -  for (tail = current_buffer->overlays_before; tail; tail = tail->next)
> +  for (tail = buffer_get_overlays (NULL, OV_BEFORE); tail; tail = tail->next)
>      {
>        ptrdiff_t startpos, endpos;
 
> @@ -2650,7 +2653,7 @@
>  	next = startpos;
>      }
 
> -  for (tail = current_buffer->overlays_after; tail; tail = tail->next)
> +  for (tail = buffer_get_overlays (NULL, OV_AFTER); tail; tail = tail->next)
>      {
>        ptrdiff_t startpos, endpos;
 
> @@ -2737,7 +2740,7 @@
>    int inhibit_storing = 0;
>    int end_is_Z = end == Z;
 
> -  for (tail = current_buffer->overlays_before; tail; tail = tail->next)
> +  for (tail = buffer_get_overlays (NULL, OV_BEFORE); tail; tail = tail->next)
>      {
>        ptrdiff_t startpos, endpos;
 
> @@ -2784,7 +2787,7 @@
>  	next = startpos;
>      }
 
> -  for (tail = current_buffer->overlays_after; tail; tail = tail->next)
> +  for (tail = buffer_get_overlays (NULL, OV_AFTER); tail; tail = tail->next)
>      {
>        ptrdiff_t startpos, endpos;
 
> @@ -2874,7 +2877,7 @@
>    Lisp_Object overlay;
>    struct Lisp_Overlay *tail;
 
> -  for (tail = current_buffer->overlays_before; tail; tail = tail->next)
> +  for (tail = buffer_get_overlays (NULL, OV_BEFORE); tail; tail = tail->next)
>      {
>        ptrdiff_t endpos;
 
> @@ -2888,7 +2891,7 @@
>  	return 1;
>      }
 
> -  for (tail = current_buffer->overlays_after; tail; tail = tail->next)
> +  for (tail = buffer_get_overlays (NULL, OV_AFTER); tail; tail = tail->next)
>      {
>        ptrdiff_t startpos;
 
> @@ -3089,7 +3092,7 @@
 
>    overlay_heads.used = overlay_heads.bytes = 0;
>    overlay_tails.used = overlay_tails.bytes = 0;
> -  for (ov = current_buffer->overlays_before; ov; ov = ov->next)
> +  for (ov = buffer_get_overlays (NULL, OV_BEFORE); ov; ov = ov->next)
>      {
>        XSETMISC (overlay, ov);
>        eassert (OVERLAYP (overlay));
> @@ -3117,7 +3120,7 @@
>  			       Foverlay_get (overlay, Qpriority),
>  			       endpos - startpos);
>      }
> -  for (ov = current_buffer->overlays_after; ov; ov = ov->next)
> +  for (ov = buffer_get_overlays (NULL, OV_AFTER); ov; ov = ov->next)
>      {
>        XSETMISC (overlay, ov);
>        eassert (OVERLAYP (overlay));
> @@ -3215,7 +3218,7 @@
>       But we use it for symmetry and in case that should cease to be true
>       with some future change.  */
>    prev = NULL;
> -  for (tail = buf->overlays_before; tail; prev = tail, tail = next)
> +  for (tail = buffer_get_overlays (buf, OV_BEFORE); tail; prev = tail, tail = next)
>      {
>        next = tail->next;
>        XSETMISC (overlay, tail);
> @@ -3234,11 +3237,11 @@
>  	  if (prev)
prev-> next = next;
>  	  else
> -	    buf->overlays_before = next;
> +	    buffer_set_overlays (buf, next, OV_BEFORE);
 
>  	  /* Search thru overlays_after for where to put it.  */
>  	  other_prev = NULL;
> -	  for (other = buf->overlays_after; other;
> +	  for (other = buffer_get_overlays (buf, OV_AFTER); other;
>  	       other_prev = other, other = other->next)
>  	    {
>  	      Lisp_Object otherbeg, otheroverlay;
> @@ -3256,7 +3259,7 @@
>  	  if (other_prev)
other_prev-> next = tail;
>  	  else
> -	    buf->overlays_after = tail;
> +	    buffer_set_overlays (buf, tail, OV_AFTER);
>  	  tail = prev;
>  	}
>        else
> @@ -3268,7 +3271,7 @@
 
>    /* See if anything in overlays_after should be in overlays_before.  */
>    prev = NULL;
> -  for (tail = buf->overlays_after; tail; prev = tail, tail = next)
> +  for (tail = buffer_get_overlays (buf, OV_AFTER); tail; prev = tail, tail = next)
>      {
>        next = tail->next;
>        XSETMISC (overlay, tail);
> @@ -3292,11 +3295,11 @@
>  	  if (prev)
prev-> next = next;
>  	  else
> -	    buf->overlays_after = next;
> +	    buffer_set_overlays (buf, next, OV_AFTER);
 
>  	  /* Search thru overlays_before for where to put it.  */
>  	  other_prev = NULL;
> -	  for (other = buf->overlays_before; other;
> +	  for (other = buffer_get_overlays (buf, OV_BEFORE); other;
>  	       other_prev = other, other = other->next)
>  	    {
>  	      Lisp_Object otherend, otheroverlay;
> @@ -3314,7 +3317,7 @@
>  	  if (other_prev)
other_prev-> next = tail;
>  	  else
> -	    buf->overlays_before = tail;
> +	    buffer_set_overlays (buf, tail, OV_BEFORE);
>  	  tail = prev;
>  	}
>      }
> @@ -3367,7 +3370,7 @@
>       assigned.  */
>    struct Lisp_Overlay *beforep = NULL, *afterp = NULL;
>    /* 'Parent', likewise, indicates a cons cell or
> -     current_buffer->overlays_before or overlays_after, depending
> +     before or after overlays list, depending
>       which loop we're in.  */
>    struct Lisp_Overlay *tail, *parent;
>    ptrdiff_t startpos, endpos;
> @@ -3379,7 +3382,7 @@
>       (after_list) if it is, is still uninitialized.  So it's not a bug
>       that before_list isn't initialized, although it may look
>       strange.  */
> -  for (parent = NULL, tail = current_buffer->overlays_before; tail;)
> +  for (parent = NULL, tail = buffer_get_overlays (NULL, OV_BEFORE); tail;)
>      {
>        XSETMISC (overlay, tail);
 
> @@ -3419,7 +3422,7 @@
>  	      beforep = tail;
>  	    }
>  	  if (!parent)
> -	    current_buffer->overlays_before = tail->next;
> +	    buffer_set_overlays (NULL, tail->next, OV_BEFORE);
>  	  else
parent-> next = tail->next;
>  	  tail = tail->next;
> @@ -3427,7 +3430,7 @@
>        else
>  	parent = tail, tail = parent->next;
>      }
> -  for (parent = NULL, tail = current_buffer->overlays_after; tail;)
> +  for (parent = NULL, tail = buffer_get_overlays (NULL, OV_AFTER); tail;)
>      {
>        XSETMISC (overlay, tail);
 
> @@ -3465,7 +3468,7 @@
>  	      beforep = tail;
>  	    }
>  	  if (!parent)
> -	    current_buffer->overlays_after = tail->next;
> +	    buffer_set_overlays (NULL, tail->next, OV_AFTER);
>  	  else
parent-> next = tail->next;
>  	  tail = tail->next;
> @@ -3478,15 +3481,15 @@
>       and let the recenter function make it sane again.  */
>    if (beforep)
>      {
> -      beforep->next = current_buffer->overlays_before;
> -      current_buffer->overlays_before = before_list;
> +      beforep->next = buffer_get_overlays (NULL, OV_BEFORE);
> +      buffer_set_overlays (NULL, before_list, OV_BEFORE);
>      }
>    recenter_overlay_lists (current_buffer, current_buffer->overlay_center);
 
>    if (afterp)
>      {
> -      afterp->next = current_buffer->overlays_after;
> -      current_buffer->overlays_after = after_list;
> +      afterp->next = buffer_get_overlays (NULL, OV_AFTER);
> +      buffer_set_overlays (NULL, after_list, OV_AFTER);
>      }
>    recenter_overlay_lists (current_buffer, current_buffer->overlay_center);
>  }
> @@ -3507,7 +3510,8 @@
>  fix_overlays_before (struct buffer *bp, ptrdiff_t prev, ptrdiff_t pos)
>  {
>    /* If parent is nil, replace overlays_before; otherwise, parent->next.  */
> -  struct Lisp_Overlay *tail = bp->overlays_before, *parent = NULL, *right_pair;
> +  struct Lisp_Overlay *tail = buffer_get_overlays (bp, OV_BEFORE);
> +  struct Lisp_Overlay *parent = NULL, *right_pair;
>    Lisp_Object tem;
>    ptrdiff_t end IF_LINT (= 0);
 
> @@ -3562,8 +3566,8 @@
>  	     and link it into the right place.  */
>  	  if (!right_pair)
>  	    {
> -	      found->next = bp->overlays_before;
> -	      bp->overlays_before = found;
> +	      found->next = buffer_get_overlays (bp, OV_BEFORE);
> +	      buffer_set_overlays (bp, found, OV_BEFORE);
>  	    }
>  	  else
>  	    {
> @@ -3639,15 +3643,15 @@
>    end = OVERLAY_END (overlay);
>    if (OVERLAY_POSITION (end) < b->overlay_center)
>      {
> -      if (b->overlays_after)
> -	XOVERLAY (overlay)->next = b->overlays_after;
> -      b->overlays_after = XOVERLAY (overlay);
> +      if (buffer_get_overlays (b, OV_AFTER))
> +	XOVERLAY (overlay)->next = buffer_get_overlays (b, OV_AFTER);
> +      buffer_set_overlays (b, XOVERLAY (overlay), OV_AFTER);
>      }
>    else
>      {
> -      if (b->overlays_before)
> -	XOVERLAY (overlay)->next = b->overlays_before;
> -      b->overlays_before = XOVERLAY (overlay);
> +      if (buffer_get_overlays (b, OV_BEFORE))
> +	XOVERLAY (overlay)->next = buffer_get_overlays (b, OV_BEFORE);
> +      buffer_set_overlays (b, XOVERLAY (overlay), OV_BEFORE);
>      }
 
>    /* This puts it in the right list, and in the right order.  */
> @@ -3705,6 +3709,19 @@
>    return list;
>  }
 
> +/* Remove OVERLAY from both overlay lists of B.  */
> +
> +static void
> +unchain_both (struct buffer *b, Lisp_Object overlay)
> +{
> +  struct Lisp_Overlay *ov = XOVERLAY (overlay);
> +
> +  buffer_set_overlays 
> +    (b, unchain_overlay (buffer_get_overlays (b, OV_BEFORE), ov), OV_BEFORE);
> +  buffer_set_overlays
> +    (b, unchain_overlay (buffer_get_overlays (b, OV_AFTER), ov), OV_AFTER);
> +}
> +
>  DEFUN ("move-overlay", Fmove_overlay, Smove_overlay, 3, 4, 0,
>         doc: /* Set the endpoints of OVERLAY to BEG and END in BUFFER.
>  If BUFFER is omitted, leave OVERLAY in the same buffer it inhabits now.
> @@ -3755,10 +3772,7 @@
>        o_beg = OVERLAY_POSITION (OVERLAY_START (overlay));
>        o_end = OVERLAY_POSITION (OVERLAY_END (overlay));
 
> -      ob->overlays_before =
> -        unchain_overlay (ob->overlays_before, XOVERLAY (overlay));
> -      ob->overlays_after =
> -        unchain_overlay (ob->overlays_after, XOVERLAY (overlay));
> +      unchain_both (ob, overlay);
>        eassert (XOVERLAY (overlay)->next == NULL);
>      }
 
> @@ -3799,13 +3813,13 @@
>       wrong list.  */
>    if (n_end < b->overlay_center)
>      {
> -      XOVERLAY (overlay)->next = b->overlays_after;
> -      b->overlays_after = XOVERLAY (overlay);
> +      XOVERLAY (overlay)->next = buffer_get_overlays (b, OV_AFTER);
> +      buffer_set_overlays (b, XOVERLAY (overlay), OV_AFTER);
>      }
>    else
>      {
> -      XOVERLAY (overlay)->next = b->overlays_before;
> -      b->overlays_before = XOVERLAY (overlay);
> +      XOVERLAY (overlay)->next = buffer_get_overlays (b, OV_BEFORE);
> +      buffer_set_overlays (b, XOVERLAY (overlay), OV_BEFORE);
>      }
 
>    /* This puts it in the right list, and in the right order.  */
> @@ -3831,10 +3845,7 @@
>    b = XBUFFER (buffer);
>    specbind (Qinhibit_quit, Qt);
 
> -  b->overlays_before
> -    = unchain_overlay (b->overlays_before, XOVERLAY (overlay));
> -  b->overlays_after
> -    = unchain_overlay (b->overlays_after, XOVERLAY (overlay));
> +  unchain_both (b, overlay);
>    eassert (XOVERLAY (overlay)->next == NULL);
 
>    drop_overlay (b, XOVERLAY (overlay));
> @@ -4033,16 +4044,19 @@
>  {
>    struct Lisp_Overlay *ol;
>    Lisp_Object before = Qnil, after = Qnil, tmp;
> -  for (ol = current_buffer->overlays_before; ol; ol = ol->next)
> +
> +  for (ol = buffer_get_overlays (NULL, OV_BEFORE); ol; ol = ol->next)
>      {
>        XSETMISC (tmp, ol);
>        before = Fcons (tmp, before);
>      }
> -  for (ol = current_buffer->overlays_after; ol; ol = ol->next)
> +
> +  for (ol = buffer_get_overlays (NULL, OV_AFTER); ol; ol = ol->next)
>      {
>        XSETMISC (tmp, ol);
>        after = Fcons (tmp, after);
>      }
> +
>    return Fcons (Fnreverse (before), Fnreverse (after));
>  }
 
> @@ -4182,7 +4196,7 @@
>        /* We are being called before a change.
>  	 Scan the overlays to find the functions to call.  */
>        last_overlay_modification_hooks_used = 0;
> -      for (tail = current_buffer->overlays_before; tail; tail = tail->next)
> +      for (tail = buffer_get_overlays (NULL, OV_BEFORE); tail; tail = tail->next)
>  	{
>  	  ptrdiff_t startpos, endpos;
>  	  Lisp_Object ostart, oend;
> @@ -4219,7 +4233,7 @@
>  	    }
>  	}
 
> -      for (tail = current_buffer->overlays_after; tail; tail = tail->next)
> +      for (tail = buffer_get_overlays (NULL, OV_AFTER); tail; tail = tail->next)
>  	{
>  	  ptrdiff_t startpos, endpos;
>  	  Lisp_Object ostart, oend;
> @@ -4311,7 +4325,7 @@
 
>    hit_list = Qnil;
>    if (pos <= current_buffer->overlay_center)
> -    for (tail = current_buffer->overlays_before; tail; tail = tail->next)
> +    for (tail = buffer_get_overlays (NULL, OV_BEFORE); tail; tail = tail->next)
>        {
>  	ptrdiff_t endpos;
>  	XSETMISC (overlay, tail);
> @@ -4323,7 +4337,7 @@
>  	  hit_list = Fcons (overlay, hit_list);
>        }
>    else
> -    for (tail = current_buffer->overlays_after; tail; tail = tail->next)
> +    for (tail = buffer_get_overlays (NULL, OV_AFTER); tail; tail = tail->next)
>        {
>  	ptrdiff_t startpos;
>  	XSETMISC (overlay, tail);

> === modified file 'src/buffer.h'
> --- a/src/buffer.h	2012-08-08 12:12:40 +0000
> +++ b/src/buffer.h	2012-08-08 14:47:11 +0000
> @@ -945,6 +945,52 @@
>        }									\
>    } while (0)
 
> +enum overlay_type
> +{
> +  OV_BEFORE,
> +  OV_AFTER
> +};
> +
> +/* Get overlay list of type T and belonging to B.  */
> +
> +BUFFER_INLINE struct Lisp_Overlay *
> +buffer_get_overlays (struct buffer *b, enum overlay_type t)
> +{
> +  if (!b)
> +    b = current_buffer;
> +  if (t == OV_BEFORE)
> +    return b->overlays_before;
> +  else if (t == OV_AFTER)
> +    return b->overlays_after;
> +  else
> +    abort ();
> +}
> +
> +/* Set overlay list of type T as belonging to B.  */
> +
> +BUFFER_INLINE void
> +buffer_set_overlays (struct buffer *b, struct Lisp_Overlay *o,
> +		     enum overlay_type t)
> +{
> +  if (!b)
> +    b = current_buffer;
> +  if (t == OV_BEFORE)
> +    b->overlays_before = o;
> +  else if (t == OV_AFTER)
> +    b->overlays_after = o;
> +  else
> +    abort ();
> +}
> +
> +/* Non-zero if current buffer has overlays.  */
> +
> +BUFFER_INLINE int
> +buffer_has_overlays (void)
> +{
> +  return buffer_get_overlays (current_buffer, OV_BEFORE)
> +    || buffer_get_overlays (current_buffer, OV_AFTER);
> +}
> +
>  extern Lisp_Object Qbefore_change_functions;
>  extern Lisp_Object Qafter_change_functions;
>  extern Lisp_Object Qfirst_change_hook;

> === modified file 'src/editfns.c'
> --- a/src/editfns.c	2012-08-08 12:12:40 +0000
> +++ b/src/editfns.c	2012-08-08 14:47:11 +0000
> @@ -310,7 +310,7 @@
>    ptrdiff_t startpos, endpos;
>    ptrdiff_t idx = 0;
 
> -  for (tail = current_buffer->overlays_before; tail; tail = tail->next)
> +  for (tail = buffer_get_overlays (NULL, OV_BEFORE); tail; tail = tail->next)
>      {
>        XSETMISC (overlay, tail);
 
> @@ -329,7 +329,7 @@
>  	}
>      }
 
> -  for (tail = current_buffer->overlays_after; tail; tail = tail->next)
> +  for (tail = buffer_get_overlays (NULL, OV_AFTER); tail; tail = tail->next)
>      {
>        XSETMISC (overlay, tail);
 

> === modified file 'src/fileio.c'
> --- a/src/fileio.c	2012-08-08 12:12:40 +0000
> +++ b/src/fileio.c	2012-08-08 14:47:11 +0000
> @@ -3490,8 +3490,8 @@
>  		  BVAR (buf, read_only) = Qnil;
>  		  BVAR (buf, filename) = Qnil;
>  		  BVAR (buf, undo_list) = Qt;
> -		  eassert (buf->overlays_before == NULL);
> -		  eassert (buf->overlays_after == NULL);
> +		  eassert (buffer_get_overlays (buf, OV_BEFORE) == NULL);
> +		  eassert (buffer_get_overlays (buf, OV_AFTER) == NULL);
 
>  		  set_buffer_internal (buf);
>  		  Ferase_buffer ();

> === modified file 'src/indent.c'
> --- a/src/indent.c	2012-08-08 12:12:40 +0000
> +++ b/src/indent.c	2012-08-08 14:47:11 +0000
> @@ -337,8 +337,7 @@
>    /* If the buffer has overlays, text properties,
>       or multibyte characters, use a more general algorithm.  */
>    if (buffer_get_intervals (current_buffer)
> -      || current_buffer->overlays_before
> -      || current_buffer->overlays_after
> +      || buffer_has_overlays ()
>        || Z != Z_BYTE)
>      return current_column_1 ();
 

> === modified file 'src/insdel.c'
> --- a/src/insdel.c	2012-08-08 12:12:40 +0000
> +++ b/src/insdel.c	2012-08-08 14:47:11 +0000
> @@ -1993,7 +1993,7 @@
>        XSETCDR (rvoe_arg, Qt);
>      }
 
> -  if (current_buffer->overlays_before || current_buffer->overlays_after)
> +  if (buffer_has_overlays ())
>      {
>        PRESERVE_VALUE;
>        report_overlay_modification (FETCH_START, FETCH_END, 0,
> @@ -2029,8 +2029,7 @@
>       just record the args that we were going to use.  */
>    if (! NILP (Vcombine_after_change_calls)
>        && NILP (Vbefore_change_functions)
> -      && !current_buffer->overlays_before
> -      && !current_buffer->overlays_after)
> +      && !buffer_has_overlays ())
>      {
>        Lisp_Object elt;
 
> @@ -2072,7 +2071,7 @@
>        XSETCDR (rvoe_arg, Qt);
>      }
 
> -  if (current_buffer->overlays_before || current_buffer->overlays_after)
> +  if (buffer_has_overlays ())
>      report_overlay_modification (make_number (charpos),
>  				 make_number (charpos + lenins),
>  				 1,

> === modified file 'src/intervals.c'
> --- a/src/intervals.c	2012-08-08 12:12:40 +0000
> +++ b/src/intervals.c	2012-08-08 14:47:11 +0000
> @@ -1870,8 +1870,7 @@
>       whether or not there are intervals in the buffer.  */
>    eassert (charpos <= ZV && charpos >= BEGV);
 
> -  have_overlays = (current_buffer->overlays_before
> -		   || current_buffer->overlays_after);
> +  have_overlays = buffer_has_overlays ();
 
>    /* If we have no text properties and overlays,
>       then we can do it quickly.  */

> === modified file 'src/print.c'
> --- a/src/print.c	2012-08-08 10:23:04 +0000
> +++ b/src/print.c	2012-08-08 14:47:11 +0000
> @@ -498,8 +498,8 @@
>    BVAR (current_buffer, read_only) = Qnil;
>    BVAR (current_buffer, filename) = Qnil;
>    BVAR (current_buffer, undo_list) = Qt;
> -  eassert (current_buffer->overlays_before == NULL);
> -  eassert (current_buffer->overlays_after == NULL);
> +  eassert (buffer_get_overlays (NULL, OV_BEFORE) == NULL);
> +  eassert (buffer_get_overlays (NULL, OV_AFTER) == NULL);
>    BVAR (current_buffer, enable_multibyte_characters)
>      = BVAR (&buffer_defaults, enable_multibyte_characters);
>    specbind (Qinhibit_read_only, Qt);

> === modified file 'src/xdisp.c'
> --- a/src/xdisp.c	2012-08-08 06:11:29 +0000
> +++ b/src/xdisp.c	2012-08-08 14:47:11 +0000
> @@ -5445,7 +5445,7 @@
>    while (0)
 
>    /* Process overlay before the overlay center.  */
> -  for (ov = current_buffer->overlays_before; ov; ov = ov->next)
> +  for (ov = buffer_get_overlays (NULL, OV_BEFORE); ov; ov = ov->next)
>      {
>        XSETMISC (overlay, ov);
>        eassert (OVERLAYP (overlay));
> @@ -5485,7 +5485,7 @@
>      }
 
>    /* Process overlays after the overlay center.  */
> -  for (ov = current_buffer->overlays_after; ov; ov = ov->next)
> +  for (ov = buffer_get_overlays (NULL, OV_AFTER); ov; ov = ov->next)
>      {
>        XSETMISC (overlay, ov);
>        eassert (OVERLAYP (overlay));


> _______________________________________________
> Emacs-diffs mailing list
> Emacs-diffs@gnu.org
> https://lists.gnu.org/mailman/listinfo/emacs-diffs

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109512: Inline functions to examine and change buffer overlays.

[Dmitry Antipov]

On 08/08/2012 07:45 PM, Stefan Monnier wrote:

> Can we pause for a moment?
> As I mentioned to you earlier, you should post such patches for review
> before installing them.
> And explain why you think they're good.
> E.g. replacing buffer->overlays_before by "buffer_get_overlays (buffer,
> OV_BEFORE)" is far from "obviously good" in my eyes.
> If you intend to make those field accesses into something more complex,
> then please say so and explain why.  If not, then please stick to
> "foo->bar", as agreed for the FGET case.

I think that it's reasonable to have just one chain of overlays per
buffer, much like the markers and intervals chains per buffer text.
And buffer_get_overlays(b, WHAT) is expected to simplify transition
to this possible change.

Dmitry

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109512: Inline functions to examine and change buffer overlays.

[Stefan Monnier]

> I think that it's reasonable to have just one chain of overlays per
> buffer, much like the markers and intervals chains per buffer text.

Why?
The point of having 2 is that they're sorted in opposite order, so that
finding overlays close to the division point is faster than O(n).

I think the only meaningful improvement for overlays would be to move
them to the interval tree, thus replacing the O(n) worst case by an
O(log n) worst case for most operations.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109512: Inline functions to examine and change buffer overlays.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Wed, 08 Aug 2012 14:05:12 -0400
> Cc: emacs-devel@gnu.org
> 
> > I think that it's reasonable to have just one chain of overlays per
> > buffer, much like the markers and intervals chains per buffer text.
> 
> Why?
> The point of having 2 is that they're sorted in opposite order, so that
> finding overlays close to the division point is faster than O(n).

FWIW, the display engine uses this division quite a lot.  If we end up
removing it, I suggest to time the code with and without it, e.g. by
timing some modes that are heavy users of overlays.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109512: Inline functions to examine and change buffer overlays.

[Stefan Monnier]

> FWIW, the display engine uses this division quite a lot.  If we end up
> removing it, I suggest to time the code with and without it, e.g. by
> timing some modes that are heavy users of overlays.

I don't think we need timings.  We've already had to add some calls to
overlay-recenter following bug-reports of pathologically slow
operations, so there's no doubt that the performance impact would be
a problem.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109512: Inline functions to examine and change buffer overlays.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Wed, 08 Aug 2012 15:47:58 -0400
> Cc: dmantipov@yandex.ru, emacs-devel@gnu.org
> 
> > FWIW, the display engine uses this division quite a lot.  If we end up
> > removing it, I suggest to time the code with and without it, e.g. by
> > timing some modes that are heavy users of overlays.
> 
> I don't think we need timings.

Not if we aren't getting rid of overlay-recenter, we don't ;-)

Markers/intervals/overlays + trees

[Dmitry Antipov]

On 08/09/2012 12:44 AM, Eli Zaretskii wrote:
>> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
>> Date: Wed, 08 Aug 2012 15:47:58 -0400
>> Cc: dmantipov@yandex.ru, emacs-devel@gnu.org
>>
>>> FWIW, the display engine uses this division quite a lot.  If we end up
>>> removing it, I suggest to time the code with and without it, e.g. by
>>> timing some modes that are heavy users of overlays.
>>
>> I don't think we need timings.
>
> Not if we aren't getting rid of overlay-recenter, we don't ;-)

The more I do different things around buffers and markers/intervals/overlays,
the more I think that the last three ones represents nearly the same thing,
i.e. "buffer range with properties" (marker is the range of length 0 (or 1,
that's may be the question), and without properties). Is it reasonable/
possible/feasible to generalize them into the only type and use it everywhere?

If not, shouldn't markers and overlays be chained into double-linked lists
instead of single-linked, for the sake of fast unchain/re-chain and in-place sort?

Finally, what about an idea to generalize red-black tree from alloc.c and
use it everywhere when O(log(n)) data structure is needed? I suppose that we
can even avoid our own tree implementation while compiling for GNU/Linux because
glibc trees (tsearch/tfind/etc.) are balanced and good enough in general.

Dmitry

Re: Markers/intervals/overlays + trees

[Eli Zaretskii]

> Date: Thu, 09 Aug 2012 07:53:48 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Eli Zaretskii <eliz@gnu.org>, 
>  Stefan Monnier <monnier@IRO.UMontreal.CA>
> 
> The more I do different things around buffers and markers/intervals/overlays,
> the more I think that the last three ones represents nearly the same thing,
> i.e. "buffer range with properties" (marker is the range of length 0 (or 1,
> that's may be the question), and without properties).

They are not necessarily as similar as you make them sound.  I don't
know if the dissimilarities contradict your idea, but they should be
kept in mind:

 . Intervals are basis for text properties, and text properties cannot
   overlap, in the sense that 2 text properties with same property but
   different values cannot span overlapping regions of text.  By
   contrast, overlays with the same property but different values
   _can_ overlap.

 . Text properties can be attached to strings, not just buffers.  The
   other two kinds cannot.

> Is it reasonable/ possible/feasible to generalize them into the only
> type and use it everywhere?

What would be the gains from such a generalization?

> If not, shouldn't markers and overlays be chained into double-linked lists
> instead of single-linked, for the sake of fast unchain/re-chain and in-place sort?

My gut feeling is that the most important operation is to find an
overlay covering the some buffer position, or the next/previous
overlay from a given position.  Optimization efforts should be
directed to these operations first and foremost.

> Finally, what about an idea to generalize red-black tree from alloc.c and
> use it everywhere when O(log(n)) data structure is needed?

Aren't insertion and deletion more expensive in red-black trees than
in "regular" binary trees?  If they are, this will hurt text
properties performance.  Buffers with an enormous amount of text
properties are quite frequent.

As another data point, the XEmacs "extents", which are (AFAIK) a kind
of text properties and overlays in the same feature, are not
implemented on top of trees.  I dare to assume that this is not
because the designers didn't know about trees.

IOW, I think  little more analysis and initial design is needed before
we can assess the alternatives.

Re: Markers/intervals/overlays + trees

[Stefan Monnier]

> The more I do different things around buffers and
> markers/intervals/overlays, the more I think that the last three ones
> represents nearly the same thing, i.e. "buffer range with properties"
> (marker is the range of length 0 (or 1, that's may be the question),

Definitely 0.

> and without properties). Is it reasonable/ possible/feasible to
> generalize them into the only type and use it everywhere?

As mentioned elsewhere, we could/should move overlays to the tree
currently used for text-properties (using the "augmented tree" approach
described in http://en.wikipedia.org/wiki/Interval_tree, and using the
"interval tree" of text-properties as the simple underlying balanced
binary tree).

This wouldn't quite merge text-properties and overlays (the two concepts
are subtly different), but it would bring the implementation of the two
closer and should make overlays a lot more scalable/efficient.

I suspect that once this is done, markers wouldn't matter much any more
(because overlays wouldn't use them internally), so we could keep the
current implementation or turn them into "degenerate overlays".

> If not, shouldn't markers and overlays be chained into double-linked
> lists instead of single-linked, for the sake of fast unchain/re-chain
> and in-place sort?

Not sure it's worth the trouble:
- doubly-linked lists wouldn't speed up chaining.
- while unchaining would be faster, this is normally not a common
  operation (hopefully most markers should be unchained lazily in batch
  by the GC, which can do it efficiently).
  Of course, this is only true of real markers, not of overlay's markers
  since these aren't implicitly reclaimed by the GC.
- sorting would slow down insertion and would only speed up other
  operations by a factor 2 (you only need to traverse half the list on
  average).  When the list is long enough to be a problem, a factor 2 is
  not sufficient, we need algorithmic improvement.

> Finally, what about an idea to generalize red-black tree from alloc.c
> and use it everywhere when O(log(n)) data structure is needed?
> I suppose that we can even avoid our own tree implementation while
> compiling for GNU/Linux because glibc trees (tsearch/tfind/etc.) are
> balanced and good enough in general.

If we have to provide our own implementation in some cases, then we may
as well use it everywhere.  OTOH we could maybe use Glib's trees.
It would probably be fairly easy to do for alloc.c's redblack trees, but
changing text-properties's intervals trees to some "standard" balanced
tree library would probably take more work.

Also it would be less efficient since every node would be split into the
"tree node" and the "value node".


        Stefan