bug#46915: [PATCH] Remove unecessary change_req arg from overlays_at()

bug#46915: [PATCH] Remove unecessary change_req arg from overlays_at()

[Matt Armstrong]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

Tags: patch

I was scratching my head trying to figure out why overlays_at() had such
a complex function signature.  It turns out that it does not need to be
so complex now and, I think, it never did.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-unecessary-change_req-arg-from-overlays_at.patch --]
[-- Type: text/x-patch, Size: 6996 bytes --]

From 0337e4823b32dd15bdae1f61df12b5f68170e360 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@mdeb>
Date: Wed, 3 Mar 2021 16:31:02 -0800
Subject: [PATCH] Remove unecessary change_req arg from overlays_at()

The change_req arg was an unecessary complexity.  It only changed what
might be assigned to the *prev_ptr arg, and all callers that passed a
non-null prev_ptr also passed a true change_req.

This makes it clear that no caller desires the behavior that would
have occurred with a non-null prev_ptr and a false change_req.

For archaeologists, the above invariants appear to have been true from
the beginning, and whatever bug fixed by the commits below need not
have been controlled with a new boolean arg.  See commits
ac869cf715 ((overlays_at): Add CHANGE_REQ parameter, 2000-08-08) and
1d5f4c1de4 ((overlays_at): Only let CHANGE_REQ inhibit an assignment
of startpos to prev when startpos == pos, 2000-10-25).

* src/buffer.c (overlays_at):
* src/buffer.h (GET_OVERLAYS_AT): Remove the change_req arg.
* src/buffer.c (Foverlays_at):
(Fnext_overlay_change):
* src/textprop.c (get_char_property_and_overlay):
(note_mouse_highlight):
* src/xfaces.c (face_at_buffer_position):
* src/xdisp.c (next_overlay_change):
(note_mouse_highlight): Omit the change_req arg.
---
 src/buffer.c   | 13 ++++++-------
 src/buffer.h   | 11 +++++------
 src/textprop.c |  2 +-
 src/xdisp.c    |  4 ++--
 src/xfaces.c   |  2 +-
 5 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 5bd9b37702..84edf18eb0 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -2843,14 +2843,13 @@ DEFUN ("kill-all-local-variables", Fkill_all_local_variables,
    and store only as many overlays as will fit.
    But still return the total number of overlays.
 
-   If CHANGE_REQ, any position written into *PREV_PTR or
-   *NEXT_PTR is guaranteed to be not equal to POS, unless it is the
-   default (BEGV or ZV).  */
+   Any position written into *PREV_PTR or *NEXT_PTR is guaranteed to
+   be not equal to POS, unless it is the default (BEGV or ZV).  */
 
 ptrdiff_t
 overlays_at (EMACS_INT pos, bool extend, Lisp_Object **vec_ptr,
 	     ptrdiff_t *len_ptr,
-	     ptrdiff_t *next_ptr, ptrdiff_t *prev_ptr, bool change_req)
+	     ptrdiff_t *next_ptr, ptrdiff_t *prev_ptr)
 {
   ptrdiff_t idx = 0;
   ptrdiff_t len = *len_ptr;
@@ -2944,7 +2943,7 @@ overlays_at (EMACS_INT pos, bool extend, Lisp_Object **vec_ptr,
       else if (endpos < pos && endpos > prev)
 	prev = endpos;
       else if (endpos == pos && startpos > prev
-	       && (!change_req || startpos < pos))
+	       && startpos < pos)
 	prev = startpos;
     }
 
@@ -4232,7 +4231,7 @@ DEFUN ("overlays-at", Foverlays_at, Soverlays_at, 1, 2, 0,
   /* Put all the overlays we want in a vector in overlay_vec.
      Store the length in len.  */
   noverlays = overlays_at (XFIXNUM (pos), 1, &overlay_vec, &len,
-			   NULL, NULL, 0);
+			   NULL, NULL);
 
   if (!NILP (sorted))
     noverlays = sort_overlays (overlay_vec, noverlays,
@@ -4308,7 +4307,7 @@ DEFUN ("next-overlay-change", Fnext_overlay_change, Snext_overlay_change,
      Store the length in len.
      endpos gets the position where the next overlay starts.  */
   noverlays = overlays_at (XFIXNUM (pos), 1, &overlay_vec, &len,
-			   &endpos, 0, 1);
+			   &endpos, 0);
 
   /* If any of these overlays ends before endpos,
      use its ending point instead.  */
diff --git a/src/buffer.h b/src/buffer.h
index 24e9c3fcbc..a53b476d72 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -1145,7 +1145,7 @@ #define CHECK_FIXNUM_COERCE_MARKER(x) ((x) = make_fixnum (fix_position (x)))
 extern void compact_buffer (struct buffer *);
 extern void evaporate_overlays (ptrdiff_t);
 extern ptrdiff_t overlays_at (EMACS_INT, bool, Lisp_Object **,
-			      ptrdiff_t *, ptrdiff_t *, ptrdiff_t *, bool);
+			      ptrdiff_t *, ptrdiff_t *, ptrdiff_t *);
 extern ptrdiff_t sort_overlays (Lisp_Object *, ptrdiff_t, struct window *);
 extern void recenter_overlay_lists (struct buffer *, ptrdiff_t);
 extern ptrdiff_t overlay_strings (ptrdiff_t, struct window *, unsigned char **);
@@ -1195,22 +1195,21 @@ record_unwind_current_buffer (void)
 
 /* Get overlays at POSN into array OVERLAYS with NOVERLAYS elements.
    If NEXTP is non-NULL, return next overlay there.
-   See overlay_at arg CHANGE_REQ for meaning of CHRQ arg.
    This macro might evaluate its args multiple times,
-   and it treat some args as lvalues.  */
+   and it treats some args as lvalues.  */
 
-#define GET_OVERLAYS_AT(posn, overlays, noverlays, nextp, chrq)		\
+#define GET_OVERLAYS_AT(posn, overlays, noverlays, nextp)		\
   do {									\
     ptrdiff_t maxlen = 40;						\
     SAFE_NALLOCA (overlays, 1, maxlen);					\
     (noverlays) = overlays_at (posn, false, &(overlays), &maxlen,	\
-			       nextp, NULL, chrq);			\
+			       nextp, NULL);			        \
     if ((noverlays) > maxlen)						\
       {									\
 	maxlen = noverlays;						\
 	SAFE_NALLOCA (overlays, 1, maxlen);				\
 	(noverlays) = overlays_at (posn, false, &(overlays), &maxlen,	\
-				   nextp, NULL, chrq);			\
+				   nextp, NULL);			\
       }									\
   } while (false)
 
diff --git a/src/textprop.c b/src/textprop.c
index d7d6a66923..854bb1c971 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -636,7 +636,7 @@ get_char_property_and_overlay (Lisp_Object position, register Lisp_Object prop,
       set_buffer_temp (XBUFFER (object));
 
       USE_SAFE_ALLOCA;
-      GET_OVERLAYS_AT (pos, overlay_vec, noverlays, NULL, false);
+      GET_OVERLAYS_AT (pos, overlay_vec, noverlays, NULL);
       noverlays = sort_overlays (overlay_vec, noverlays, w);
 
       set_buffer_temp (obuf);
diff --git a/src/xdisp.c b/src/xdisp.c
index cc0a689ba3..f64b42f2d8 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -4070,7 +4070,7 @@ next_overlay_change (ptrdiff_t pos)
   USE_SAFE_ALLOCA;
 
   /* Get all overlays at the given position.  */
-  GET_OVERLAYS_AT (pos, overlays, noverlays, &endpos, true);
+  GET_OVERLAYS_AT (pos, overlays, noverlays, &endpos);
 
   /* If any of these overlays ends before endpos,
      use its ending point instead.  */
@@ -33729,7 +33729,7 @@ note_mouse_highlight (struct frame *f, int x, int y)
       if (BUFFERP (object))
 	{
 	  /* Put all the overlays we want in a vector in overlay_vec.  */
-	  GET_OVERLAYS_AT (pos, overlay_vec, noverlays, NULL, false);
+	  GET_OVERLAYS_AT (pos, overlay_vec, noverlays, NULL);
 	  /* Sort overlays into increasing priority order.  */
 	  noverlays = sort_overlays (overlay_vec, noverlays, w);
 	}
diff --git a/src/xfaces.c b/src/xfaces.c
index ab4440f46a..8df318e33e 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -6397,7 +6397,7 @@ face_at_buffer_position (struct window *w, ptrdiff_t pos,
   {
     ptrdiff_t next_overlay;
 
-    GET_OVERLAYS_AT (pos, overlay_vec, noverlays, &next_overlay, false);
+    GET_OVERLAYS_AT (pos, overlay_vec, noverlays, &next_overlay);
     if (next_overlay < endpos)
       endpos = next_overlay;
   }
-- 
2.30.0

bug#46915: [PATCH] Remove unecessary change_req arg from overlays_at()

[Eli Zaretskii]

> From: Matt Armstrong <matt@rfc20.org>
> Date: Wed, 03 Mar 2021 18:29:28 -0800
> 
> I was scratching my head trying to figure out why overlays_at() had such
> a complex function signature.  It turns out that it does not need to be
> so complex now and, I think, it never did.
> 
> >From 0337e4823b32dd15bdae1f61df12b5f68170e360 Mon Sep 17 00:00:00 2001
> From: Matt Armstrong <matt@mdeb>
> Date: Wed, 3 Mar 2021 16:31:02 -0800
> Subject: [PATCH] Remove unecessary change_req arg from overlays_at()
> 
> The change_req arg was an unecessary complexity.  It only changed what
> might be assigned to the *prev_ptr arg, and all callers that passed a
> non-null prev_ptr also passed a true change_req.
> 
> This makes it clear that no caller desires the behavior that would
> have occurred with a non-null prev_ptr and a false change_req.
> 
> For archaeologists, the above invariants appear to have been true from
> the beginning, and whatever bug fixed by the commits below need not
> have been controlled with a new boolean arg.  See commits
> ac869cf715 ((overlays_at): Add CHANGE_REQ parameter, 2000-08-08) and
> 1d5f4c1de4 ((overlays_at): Only let CHANGE_REQ inhibit an assignment
> of startpos to prev when startpos == pos, 2000-10-25).

Could you please tell what are the benefits of this change?  The
signature may be complex, but it doesn't currently cause any problems,
does it?  I'd prefer not to change code that is working and causes no
trouble.

Thanks.

bug#46915: [PATCH] Remove unecessary change_req arg from overlays_at()

[Matt Armstrong]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matt Armstrong <matt@rfc20.org>
>> Date: Wed, 03 Mar 2021 18:29:28 -0800
>> 
>> I was scratching my head trying to figure out why overlays_at() had such
>> a complex function signature.  It turns out that it does not need to be
>> so complex now and, I think, it never did.
>> 
>> >From 0337e4823b32dd15bdae1f61df12b5f68170e360 Mon Sep 17 00:00:00 2001
>> From: Matt Armstrong <matt@mdeb>
>> Date: Wed, 3 Mar 2021 16:31:02 -0800
>> Subject: [PATCH] Remove unecessary change_req arg from overlays_at()
>> 
>> The change_req arg was an unecessary complexity.  It only changed what
>> might be assigned to the *prev_ptr arg, and all callers that passed a
>> non-null prev_ptr also passed a true change_req.
>> 
>> This makes it clear that no caller desires the behavior that would
>> have occurred with a non-null prev_ptr and a false change_req.
>> 
>> For archaeologists, the above invariants appear to have been true from
>> the beginning, and whatever bug fixed by the commits below need not
>> have been controlled with a new boolean arg.  See commits
>> ac869cf715 ((overlays_at): Add CHANGE_REQ parameter, 2000-08-08) and
>> 1d5f4c1de4 ((overlays_at): Only let CHANGE_REQ inhibit an assignment
>> of startpos to prev when startpos == pos, 2000-10-25).
>
> Could you please tell what are the benefits of this change?  The
> signature may be complex, but it doesn't currently cause any problems,
> does it?  I'd prefer not to change code that is working and causes no
> trouble.

I do agree that changing code should serve some purpose.

I have been looking at the performance problems related to overlays,
mentioned in etc/TODO.  I know that this has been worked on by multiple
people over the years, but never successfully completed.  I have seen
the features/noverlay branch but have not yet convinced myself that the
code there is the right thing.  You could say that I am basically doing
a thorough code review of that approach.  I have contacted all the
people who have formerly worked on this and all of them have no plans to
resume their work, so there is space for me to do this.

The first thing I do on a project like this is determine the
functionality the current thing is providing to the rest of the system.
In other words, what is its "real" API, and how does that API differ
from its "advertised" API?

So, the benefit of this change can be summarized:

 - We don't think the current overlay implementation is necessarily the
   right thing, so we can presume that this area will change at some
   point.

 - If we simplify the API, future changes will be easier to review.  In
   particular, nobody will ever have to think about callers that want a
   *PREV_PTR value with a false CHANGE_REQ, and no future implementer
   will feel like they need to provide that functionality.

bug#46915: [PATCH] Remove unecessary change_req arg from overlays_at()

[Eli Zaretskii]

> From: Matt Armstrong <matt@rfc20.org>
> Cc: 46915@debbugs.gnu.org
> Date: Thu, 04 Mar 2021 12:03:59 -0800
> 
> So, the benefit of this change can be summarized:
> 
>  - We don't think the current overlay implementation is necessarily the
>    right thing, so we can presume that this area will change at some
>    point.
> 
>  - If we simplify the API, future changes will be easier to review.  In
>    particular, nobody will ever have to think about callers that want a
>    *PREV_PTR value with a false CHANGE_REQ, and no future implementer
>    will feel like they need to provide that functionality.

I'm okay with doing this nad similar changes as part of landing the
noverlay branch (or as part of some other similar redesign of how
overlays work in Emacs).  What I'd like to avoid is installing this
change separately, as if it alone has enough merit.  if the goal is to
replace the current implementation with a more efficient one, that's a
good change, and API simplifications could very well come with it.
But not without it and not ahead of it.

It's a bit like fixing whitespace: we don't like doing it in separate
changesets, but together with other significant changes in the same
area of the code.

So I urge you to continue the work on the noverlay branch, and if that
branch will come with this and other changes in the internal APIs,
that's fine by me.  My problem is only with installing API changes
right now, when we are not at all sure yet the overlays redesign will
indeed land any time soon.

OK?

bug#46915: [PATCH] Remove unecessary change_req arg from overlays_at()

[Matt Armstrong]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matt Armstrong <matt@rfc20.org>
>> Cc: 46915@debbugs.gnu.org
>> Date: Thu, 04 Mar 2021 12:03:59 -0800
>> 
>> So, the benefit of this change can be summarized:
>> 
>>  - We don't think the current overlay implementation is necessarily the
>>    right thing, so we can presume that this area will change at some
>>    point.
>> 
>>  - If we simplify the API, future changes will be easier to review.  In
>>    particular, nobody will ever have to think about callers that want a
>>    *PREV_PTR value with a false CHANGE_REQ, and no future implementer
>>    will feel like they need to provide that functionality.
>
> I'm okay with doing this nad similar changes as part of landing the
> noverlay branch (or as part of some other similar redesign of how
> overlays work in Emacs).  What I'd like to avoid is installing this
> change separately, as if it alone has enough merit.  if the goal is to
> replace the current implementation with a more efficient one, that's a
> good change, and API simplifications could very well come with it.
> But not without it and not ahead of it.
>
> It's a bit like fixing whitespace: we don't like doing it in separate
> changesets, but together with other significant changes in the same
> area of the code.
>
> So I urge you to continue the work on the noverlay branch, and if that
> branch will come with this and other changes in the internal APIs,
> that's fine by me.  My problem is only with installing API changes
> right now, when we are not at all sure yet the overlays redesign will
> indeed land any time soon.
>
> OK?

I'm happy to defer this for now.

I might come back and ask that this change go in before doing actual
surgery on the overlay implementation.  In the past, on other projects,
I have found it worthwhile to first simplify APIs, and assert various
assumed invariants more rigidly, and let those changes bake for a while,
before beginning to modify implementations.  But, I would agree that
those kinds of changes are best done according to some sort of generally
agreed upon plan, not just ad hoc.

bug#46915: [PATCH] Remove unecessary change_req arg from overlays_at()

[Eli Zaretskii]

> From: Matt Armstrong <matt@rfc20.org>
> Cc: 46915@debbugs.gnu.org
> Date: Sat, 06 Mar 2021 16:03:14 -0800
> 
> > So I urge you to continue the work on the noverlay branch, and if that
> > branch will come with this and other changes in the internal APIs,
> > that's fine by me.  My problem is only with installing API changes
> > right now, when we are not at all sure yet the overlays redesign will
> > indeed land any time soon.
> >
> > OK?
> 
> I'm happy to defer this for now.

Thank you.  And thanks for your interest to this area in Emacs, whose
reimplementation is long overdue.

> I might come back and ask that this change go in before doing actual
> surgery on the overlay implementation.  In the past, on other projects,
> I have found it worthwhile to first simplify APIs, and assert various
> assumed invariants more rigidly, and let those changes bake for a while,
> before beginning to modify implementations.  But, I would agree that
> those kinds of changes are best done according to some sort of generally
> agreed upon plan, not just ad hoc.

If you do the work on a feature branch, the order is entirely up to
you.  What I'd like to avoid is to have such changes land on master
without being sure they will be reasonably closely followed by "the
real thing".  We had our share of disappointments with such changes in
the past, and I'd like to avoid repeating such mistakes.