Dead or unused face handling code

Dead or unused face handling code

[Dmitry Antipov]

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

I've found a few dummy/unused bits around face_at_xxx functions, and running
with these patches (which see) for a few days without any problems. But since
redisplay is very tricky, I don't feel brave enough to install them without
having some feedback first...

Dmitry

[-- Attachment #2: 0001-Simplify-face_at_string_position-and-related-users.patch --]
[-- Type: text/x-diff, Size: 9581 bytes --]

From 40a8513ac2f94130ca79a1ddfa956b5f01370282 Mon Sep 17 00:00:00 2001
From: Dmitry Antipov <dmantipov@yandex.ru>
Date: Thu, 7 Jul 2016 08:20:09 +0300
Subject: [PATCH 1/3] Simplify face_at_string_position and related users

* src/xfaces.c (face_at_string_position): Remove 4th and 5th args,
avoid call to Fnext_single_property_change because it's not actually
required by the callers.
* src/dispextern.h (face_at_string_position): Adjust prototype.
* src/font.c (font_at):
* src/xdisp.c (handle_face_prop, face_before_or_after_it_pos)
(get_next_display_element, display_string, note_mouse_highlight)
(note_mode_line_or_margin_highlight): Related users changed.
---
 src/dispextern.h |  4 ++--
 src/font.c       |  2 +-
 src/xdisp.c      | 54 +++++++++++++++++-------------------------------------
 src/xfaces.c     | 36 +++++-------------------------------
 4 files changed, 25 insertions(+), 71 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 1325ff9..922534f 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3440,8 +3440,8 @@ int face_at_buffer_position (struct window *, ptrdiff_t, ptrdiff_t *, ptrdiff_t,
                              bool, int);
 int face_for_overlay_string (struct window *, ptrdiff_t, ptrdiff_t *, ptrdiff_t,
                              bool, Lisp_Object);
-int face_at_string_position (struct window *, Lisp_Object, ptrdiff_t, ptrdiff_t,
-                             ptrdiff_t *, enum face_id, bool);
+int face_at_string_position (struct window *, Lisp_Object, ptrdiff_t,
+                             enum face_id, bool);
 int merge_faces (struct frame *, Lisp_Object, int, int);
 int compute_char_face (struct frame *, int, Lisp_Object);
 void free_all_realized_faces (Lisp_Object);
diff --git a/src/font.c b/src/font.c
index 144ba07..2abb508 100644
--- a/src/font.c
+++ b/src/font.c
@@ -3747,7 +3747,7 @@ font_at (int c, ptrdiff_t pos, struct face *face, struct window *w,
       ptrdiff_t endptr;
 
       if (STRINGP (string))
-	face_id = face_at_string_position (w, string, pos, 0, &endptr,
+	face_id = face_at_string_position (w, string, pos,
 					   DEFAULT_FACE_ID, false);
       else
 	face_id = face_at_buffer_position (w, pos, &endptr,
diff --git a/src/xdisp.c b/src/xdisp.c
index d5ffb25..f7db9de 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -3907,7 +3907,6 @@ handle_face_prop (struct it *it)
   else
     {
       int base_face_id;
-      ptrdiff_t bufpos;
       int i;
       Lisp_Object from_overlay
 	= (it->current.overlay_string_index >= 0
@@ -3936,23 +3935,18 @@ handle_face_prop (struct it *it)
 	  }
 
       if (! NILP (from_overlay))
-	{
-	  bufpos = IT_CHARPOS (*it);
-	  /* For a string from an overlay, the base face depends
-	     only on text properties and ignores overlays.  */
-	  base_face_id
-	    = face_for_overlay_string (it->w,
-				       IT_CHARPOS (*it),
-				       &next_stop,
-				       (IT_CHARPOS (*it)
-					+ TEXT_PROP_DISTANCE_LIMIT),
-				       false,
-				       from_overlay);
-	}
+	/* For a string from an overlay, the base face depends
+	   only on text properties and ignores overlays.  */
+	base_face_id
+	  = face_for_overlay_string (it->w,
+				     IT_CHARPOS (*it),
+				     &next_stop,
+				     (IT_CHARPOS (*it)
+				      + TEXT_PROP_DISTANCE_LIMIT),
+				     false,
+				     from_overlay);
       else
 	{
-	  bufpos = 0;
-
 	  /* For strings from a `display' property, use the face at
 	     IT's current buffer position as the base face to merge
 	     with, so that overlay strings appear in the same face as
@@ -3979,8 +3973,6 @@ handle_face_prop (struct it *it)
       new_face_id = face_at_string_position (it->w,
 					     it->string,
 					     IT_STRING_CHARPOS (*it),
-					     bufpos,
-					     &next_stop,
 					     base_face_id, false);
 
       /* Is this a start of a run of characters with box?  Caveat:
@@ -4046,7 +4038,7 @@ face_before_or_after_it_pos (struct it *it, bool before_p)
 
   if (STRINGP (it->string))
     {
-      ptrdiff_t bufpos, charpos;
+      ptrdiff_t charpos;
       int base_face_id;
 
       /* No face change past the end of the string (for the case
@@ -4115,19 +4107,10 @@ face_before_or_after_it_pos (struct it *it, bool before_p)
 	}
       eassert (0 <= charpos && charpos <= SCHARS (it->string));
 
-      if (it->current.overlay_string_index >= 0)
-	bufpos = IT_CHARPOS (*it);
-      else
-	bufpos = 0;
-
       base_face_id = underlying_face_id (it);
 
       /* Get the face for ASCII, or unibyte.  */
-      face_id = face_at_string_position (it->w,
-					 it->string,
-					 charpos,
-					 bufpos,
-					 &next_check_charpos,
+      face_id = face_at_string_position (it->w, it->string, charpos,
 					 base_face_id, false);
 
       /* Correct the face for charsets different from ASCII.  Do it
@@ -7288,8 +7271,8 @@ get_next_display_element (struct it *it)
 			{
 			  next_face_id
 			    = face_at_string_position (it->w, base_string,
-						       CHARPOS (pos), 0,
-						       &ignore, face_id, false);
+						       CHARPOS (pos),
+						       face_id, false);
 			  it->end_of_box_run_p
 			    = (FACE_FROM_ID (it->f, next_face_id)->box
 			       == FACE_NO_BOX);
@@ -23991,12 +23974,11 @@ display_string (const char *string, Lisp_Object lisp_string, Lisp_Object face_st
      FACE_STRING, if that's given.  */
   if (STRINGP (face_string))
     {
-      ptrdiff_t endptr;
       struct face *face;
 
       it->face_id
 	= face_at_string_position (it->w, face_string, face_string_pos,
-				   0, &endptr, it->base_face_id, false);
+				   it->base_face_id, false);
       face = FACE_FROM_ID (it->f, it->face_id);
       it->face_box_p = face->box != FACE_NO_BOX;
     }
@@ -29898,7 +29880,7 @@ note_mode_line_or_margin_highlight (Lisp_Object window, int x, int y,
 	  int gpos;
 	  int gseq_length;
 	  int total_pixel_width;
-	  ptrdiff_t begpos, endpos, ignore;
+	  ptrdiff_t begpos, endpos;
 
 	  int vpos, hpos;
 
@@ -30002,7 +29984,6 @@ note_mode_line_or_margin_highlight (Lisp_Object window, int x, int y,
 
 	  hlinfo->mouse_face_face_id = face_at_string_position (w, string,
 								charpos,
-								0, &ignore,
 								glyph->face_id,
 								true);
 	  show_mouse_face (hlinfo, DRAW_MOUSE_FACE);
@@ -30318,7 +30299,6 @@ note_mouse_highlight (struct frame *f, int x, int y)
 	      /* The mouse-highlighting comes from a display string
 		 with a mouse-face.  */
 	      Lisp_Object s, e;
-	      ptrdiff_t ignore;
 
 	      s = Fprevious_single_property_change
 		(make_number (pos + 1), Qmouse_face, object, Qnil);
@@ -30333,7 +30313,7 @@ note_mouse_highlight (struct frame *f, int x, int y)
 	      hlinfo->mouse_face_past_end = false;
 	      hlinfo->mouse_face_window = window;
 	      hlinfo->mouse_face_face_id
-		= face_at_string_position (w, object, pos, 0, &ignore,
+		= face_at_string_position (w, object, pos,
 					   glyph->face_id, true);
 	      show_mouse_face (hlinfo, DRAW_MOUSE_FACE);
 	      cursor = No_Cursor;
diff --git a/src/xfaces.c b/src/xfaces.c
index 0a1315d..6146ef5 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -6032,33 +6032,20 @@ face_for_overlay_string (struct window *w, ptrdiff_t pos,
 
 
 /* Compute the face at character position POS in Lisp string STRING on
-   window W, for ASCII characters.
+   window W, for ASCII characters.  W must display the current buffer.
 
-   If STRING is an overlay string, it comes from position BUFPOS in
-   current_buffer, otherwise BUFPOS is zero to indicate that STRING is
-   not an overlay string.  W must display the current buffer.
-   REGION_BEG and REGION_END give the start and end positions of the
-   region; both are -1 if no region is visible.
-
-   BASE_FACE_ID is the id of a face to merge with.  For strings coming
-   from overlays or the `display' property it is the face at BUFPOS.
+   BASE_FACE_ID is the id of a face to merge with.
 
    If MOUSE_P, use the character's mouse-face, not its face.
 
-   Set *ENDPTR to the next position where to check for faces in
-   STRING; -1 if the face is constant from POS to the end of the
-   string.
-
    Value is the id of the face to use.  The face returned is suitable
    for displaying ASCII characters.  */
 
 int
-face_at_string_position (struct window *w, Lisp_Object string,
-			 ptrdiff_t pos, ptrdiff_t bufpos,
-			 ptrdiff_t *endptr, enum face_id base_face_id,
-			 bool mouse_p)
+face_at_string_position (struct window *w, Lisp_Object string, ptrdiff_t pos,
+			 enum face_id base_face_id, bool mouse_p)
 {
-  Lisp_Object prop, position, end, limit;
+  Lisp_Object prop, position;
   struct frame *f = XFRAME (WINDOW_FRAME (w));
   Lisp_Object attrs[LFACE_VECTOR_SIZE];
   struct face *base_face;
@@ -6070,19 +6057,6 @@ face_at_string_position (struct window *w, Lisp_Object string,
   XSETFASTINT (position, pos);
   prop = Fget_text_property (position, prop_name, string);
 
-  /* Get the next position at which to check for faces.  Value of end
-     is nil if face is constant all the way to the end of the string.
-     Otherwise it is a string position where to check faces next.
-     Limit is the maximum position up to which to check for property
-     changes in Fnext_single_property_change.  Strings are usually
-     short, so set the limit to the end of the string.  */
-  XSETFASTINT (limit, SCHARS (string));
-  end = Fnext_single_property_change (position, prop_name, string, limit);
-  if (INTEGERP (end))
-    *endptr = XFASTINT (end);
-  else
-    *endptr = -1;
-
   base_face = FACE_FROM_ID (f, base_face_id);
 
   /* Optimize the default case that there is no face property.  */
-- 
2.9.0


[-- Attachment #3: 0002-Simplify-face_for_overlay_string-and-related-users.patch --]
[-- Type: text/x-diff, Size: 3730 bytes --]

From 4f1bdeac06f0395441dad1ffa6db577aeaf81f75 Mon Sep 17 00:00:00 2001
From: Dmitry Antipov <dmantipov@yandex.ru>
Date: Thu, 7 Jul 2016 09:24:55 +0300
Subject: [PATCH 2/3] Simplify face_for_overlay_string and related users

* src/xfaces.c (face_for_overlay_string): Remove 2nd and 3rd args,
avoid call to Fnext_single_property_change because it's not actually
required by the caller.
* src/dispextern.h (face_for_overlay_string): Adjust prototype.
* src/xdisp.c (handle_face_prop): The only user changed.
---
 src/dispextern.h |  3 +--
 src/xdisp.c      |  9 ++-------
 src/xfaces.c     | 15 ---------------
 3 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 922534f..bf44cd0 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3438,8 +3438,7 @@ void free_frame_faces (struct frame *);
 void recompute_basic_faces (struct frame *);
 int face_at_buffer_position (struct window *, ptrdiff_t, ptrdiff_t *, ptrdiff_t,
                              bool, int);
-int face_for_overlay_string (struct window *, ptrdiff_t, ptrdiff_t *, ptrdiff_t,
-                             bool, Lisp_Object);
+int face_for_overlay_string (struct window *, ptrdiff_t, bool, Lisp_Object);
 int face_at_string_position (struct window *, Lisp_Object, ptrdiff_t,
                              enum face_id, bool);
 int merge_faces (struct frame *, Lisp_Object, int, int);
diff --git a/src/xdisp.c b/src/xdisp.c
index f7db9de..790709e 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -3938,13 +3938,8 @@ handle_face_prop (struct it *it)
 	/* For a string from an overlay, the base face depends
 	   only on text properties and ignores overlays.  */
 	base_face_id
-	  = face_for_overlay_string (it->w,
-				     IT_CHARPOS (*it),
-				     &next_stop,
-				     (IT_CHARPOS (*it)
-				      + TEXT_PROP_DISTANCE_LIMIT),
-				     false,
-				     from_overlay);
+	  = face_for_overlay_string (it->w, IT_CHARPOS (*it),
+				     false, from_overlay);
       else
 	{
 	  /* For strings from a `display' property, use the face at
diff --git a/src/xfaces.c b/src/xfaces.c
index 6146ef5..41a45cf 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5981,34 +5981,21 @@ face_at_buffer_position (struct window *w, ptrdiff_t pos,
 
 int
 face_for_overlay_string (struct window *w, ptrdiff_t pos,
-			 ptrdiff_t *endptr, ptrdiff_t limit,
 			 bool mouse, Lisp_Object overlay)
 {
   struct frame *f = XFRAME (w->frame);
   Lisp_Object attrs[LFACE_VECTOR_SIZE];
   Lisp_Object prop, position;
-  ptrdiff_t endpos;
   Lisp_Object propname = mouse ? Qmouse_face : Qface;
-  Lisp_Object limit1, end;
   struct face *default_face;
 
   /* W must display the current buffer.  We could write this function
      to use the frame and buffer of W, but right now it doesn't.  */
-  /* eassert (XBUFFER (w->contents) == current_buffer); */
-
   XSETFASTINT (position, pos);
 
-  endpos = ZV;
-
   /* Get the `face' or `mouse_face' text property at POS, and
      determine the next position at which the property changes.  */
   prop = Fget_text_property (position, propname, w->contents);
-  XSETFASTINT (limit1, (limit < endpos ? limit : endpos));
-  end = Fnext_single_property_change (position, propname, w->contents, limit1);
-  if (INTEGERP (end))
-    endpos = XINT (end);
-
-  *endptr = endpos;
 
   /* Optimize common case where we can use the default face.  */
   if (NILP (prop)
@@ -6023,8 +6010,6 @@ face_for_overlay_string (struct window *w, ptrdiff_t pos,
   if (!NILP (prop))
     merge_face_ref (f, prop, attrs, true, 0);
 
-  *endptr = endpos;
-
   /* Look up a realized face with the given face attributes,
      or realize a new one for ASCII characters.  */
   return lookup_face (f, attrs);
-- 
2.9.0


[-- Attachment #4: 0003-Simplify-face_at_buffer_position-and-related-users.patch --]
[-- Type: text/x-diff, Size: 9115 bytes --]

From c482a125a9bc0f0fcf1763be9d340ec8f164ed10 Mon Sep 17 00:00:00 2001
From: Dmitry Antipov <dmantipov@yandex.ru>
Date: Thu, 7 Jul 2016 09:42:06 +0300
Subject: [PATCH 3/3] Simplify face_at_buffer_position and related users

* src/xfaces.c (face_at_buffer_position): Remove 3rd arg, avoid
call to Fnext_single_property_change because it's not actually
required by the callers.
* src/dispextern.h (face_at_buffer_position): Adjust prototype.
* src/font.c (font_at, font_range, Finternal_char_font):
* src/xdisp.c (handle_face_prop, face_before_or_after_it_pos)
(get_next_display_element, mouse_face_from_buffer_pos): Related
users changed.
---
 src/dispextern.h |  3 +--
 src/font.c       | 13 ++++---------
 src/xdisp.c      | 18 +++++-------------
 src/xfaces.c     | 35 +++--------------------------------
 4 files changed, 13 insertions(+), 56 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index bf44cd0..d8ca923 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3436,8 +3436,7 @@ int lookup_derived_face (struct frame *, Lisp_Object, int, bool);
 void init_frame_faces (struct frame *);
 void free_frame_faces (struct frame *);
 void recompute_basic_faces (struct frame *);
-int face_at_buffer_position (struct window *, ptrdiff_t, ptrdiff_t *, ptrdiff_t,
-                             bool, int);
+int face_at_buffer_position (struct window *, ptrdiff_t, ptrdiff_t, bool, int);
 int face_for_overlay_string (struct window *, ptrdiff_t, bool, Lisp_Object);
 int face_at_string_position (struct window *, Lisp_Object, ptrdiff_t,
                              enum face_id, bool);
diff --git a/src/font.c b/src/font.c
index 2abb508..17d1518 100644
--- a/src/font.c
+++ b/src/font.c
@@ -3744,14 +3744,12 @@ font_at (int c, ptrdiff_t pos, struct face *face, struct window *w,
   if (! face)
     {
       int face_id;
-      ptrdiff_t endptr;
 
       if (STRINGP (string))
 	face_id = face_at_string_position (w, string, pos,
 					   DEFAULT_FACE_ID, false);
       else
-	face_id = face_at_buffer_position (w, pos, &endptr,
-					   pos + 100, false, -1);
+	face_id = face_at_buffer_position (w, pos, pos + 100, false, -1);
       face = FACE_FROM_ID (f, face_id);
     }
   if (multibyte)
@@ -3784,7 +3782,6 @@ Lisp_Object
 font_range (ptrdiff_t pos, ptrdiff_t pos_byte, ptrdiff_t *limit,
 	    struct window *w, struct face *face, Lisp_Object string)
 {
-  ptrdiff_t ignore;
   int c;
   Lisp_Object font_object = Qnil;
 
@@ -3794,8 +3791,7 @@ font_range (ptrdiff_t pos, ptrdiff_t pos_byte, ptrdiff_t *limit,
 	{
 	  int face_id;
 
-	  face_id = face_at_buffer_position (w, pos, &ignore,
-					     *limit, false, -1);
+	  face_id = face_at_buffer_position (w, pos, *limit, false, -1);
 	  face = FACE_FROM_ID (XFRAME (w->frame), face_id);
 	}
     }
@@ -4533,7 +4529,7 @@ DEFUN ("internal-char-font", Finternal_char_font, Sinternal_char_font, 1, 2, 0,
        doc: /* For internal use only.  */)
   (Lisp_Object position, Lisp_Object ch)
 {
-  ptrdiff_t pos, pos_byte, dummy;
+  ptrdiff_t pos, pos_byte;
   int face_id;
   int c;
   struct frame *f;
@@ -4568,8 +4564,7 @@ DEFUN ("internal-char-font", Finternal_char_font, Sinternal_char_font, 1, 2, 0,
 	return Qnil;
       w = XWINDOW (window);
       f = XFRAME (w->frame);
-      face_id = face_at_buffer_position (w, pos, &dummy,
-					 pos + 100, false, -1);
+      face_id = face_at_buffer_position (w, pos, pos + 100, false, -1);
     }
   if (! CHAR_VALID_P (c))
     return Qnil;
diff --git a/src/xdisp.c b/src/xdisp.c
index 790709e..ac7f3d2 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -3860,14 +3860,11 @@ static enum prop_handled
 handle_face_prop (struct it *it)
 {
   int new_face_id;
-  ptrdiff_t next_stop;
 
   if (!STRINGP (it->string))
     {
       new_face_id
-	= face_at_buffer_position (it->w,
-				   IT_CHARPOS (*it),
-				   &next_stop,
+	= face_at_buffer_position (it->w, IT_CHARPOS (*it),
 				   (IT_CHARPOS (*it)
 				    + TEXT_PROP_DISTANCE_LIMIT),
 				   false, it->base_face_id);
@@ -4025,7 +4022,6 @@ static int
 face_before_or_after_it_pos (struct it *it, bool before_p)
 {
   int face_id, limit;
-  ptrdiff_t next_check_charpos;
   struct it it_copy;
   void *it_copy_data = NULL;
 
@@ -4197,9 +4193,7 @@ face_before_or_after_it_pos (struct it *it, bool before_p)
       eassert (BEGV <= CHARPOS (pos) && CHARPOS (pos) <= ZV);
 
       /* Determine face for CHARSET_ASCII, or unibyte.  */
-      face_id = face_at_buffer_position (it->w,
-					 CHARPOS (pos),
-					 &next_check_charpos,
+      face_id = face_at_buffer_position (it->w, CHARPOS (pos),
 					 limit, false, -1);
 
       /* Correct the face for charsets different from ASCII.  Do it
@@ -7214,7 +7208,6 @@ get_next_display_element (struct it *it)
 			   /* A string from display property.  */
 			   || it->from_disp_prop_p))
 		{
-		  ptrdiff_t ignore;
 		  int next_face_id;
 		  bool text_from_string = false;
 		  /* Normally, the next buffer location is stored in
@@ -7278,7 +7271,7 @@ get_next_display_element (struct it *it)
 		  else
 		    {
 		      next_face_id =
-			face_at_buffer_position (it->w, CHARPOS (pos), &ignore,
+			face_at_buffer_position (it->w, CHARPOS (pos),
 						 CHARPOS (pos)
 						 + TEXT_PROP_DISTANCE_LIMIT,
 						 false, -1);
@@ -28985,7 +28978,7 @@ mouse_face_from_buffer_pos (Lisp_Object window,
   struct glyph_row *first = MATRIX_FIRST_TEXT_ROW (w->current_matrix);
   struct glyph_row *r1, *r2;
   struct glyph *glyph, *end;
-  ptrdiff_t ignore, pos;
+  ptrdiff_t pos;
   int x;
 
   eassert (NILP (disp_string) || STRINGP (disp_string));
@@ -29288,8 +29281,7 @@ mouse_face_from_buffer_pos (Lisp_Object window,
 
   hlinfo->mouse_face_window = window;
   hlinfo->mouse_face_face_id
-    = face_at_buffer_position (w, mouse_charpos, &ignore,
-			       mouse_charpos + 1,
+    = face_at_buffer_position (w, mouse_charpos, mouse_charpos + 1,
 			       !hlinfo->mouse_face_hidden, -1);
   show_mouse_face (hlinfo, DRAW_MOUSE_FACE);
 }
diff --git a/src/xfaces.c b/src/xfaces.c
index 41a45cf..c401b7a 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5858,13 +5858,8 @@ compute_char_face (struct frame *f, int ch, Lisp_Object prop)
   return face_id;
 }
 
-/* Return the face ID associated with buffer position POS for
-   displaying ASCII characters.  Return in *ENDPTR the position at
-   which a different face is needed, as far as text properties and
-   overlays are concerned.  W is a window displaying current_buffer.
-
-   REGION_BEG, REGION_END delimit the region, so it can be
-   highlighted.
+/* Return the face ID associated with buffer position POS for displaying
+   ASCII characters.  W is a window displaying current_buffer.
 
    LIMIT is a position not to scan beyond.  That is to limit the time
    this function can take.
@@ -5877,8 +5872,7 @@ compute_char_face (struct frame *f, int ch, Lisp_Object prop)
    The face returned is suitable for displaying ASCII characters.  */
 
 int
-face_at_buffer_position (struct window *w, ptrdiff_t pos,
-			 ptrdiff_t *endptr, ptrdiff_t limit,
+face_at_buffer_position (struct window *w, ptrdiff_t pos, ptrdiff_t limit,
 			 bool mouse, int base_face_id)
 {
   struct frame *f = XFRAME (w->frame);
@@ -5886,26 +5880,17 @@ face_at_buffer_position (struct window *w, ptrdiff_t pos,
   Lisp_Object prop, position;
   ptrdiff_t i, noverlays;
   Lisp_Object *overlay_vec;
-  ptrdiff_t endpos;
   Lisp_Object propname = mouse ? Qmouse_face : Qface;
-  Lisp_Object limit1, end;
   struct face *default_face;
 
   /* W must display the current buffer.  We could write this function
      to use the frame and buffer of W, but right now it doesn't.  */
-  /* eassert (XBUFFER (w->contents) == current_buffer); */
 
   XSETFASTINT (position, pos);
 
-  endpos = ZV;
-
   /* Get the `face' or `mouse_face' text property at POS, and
      determine the next position at which the property changes.  */
   prop = Fget_text_property (position, propname, w->contents);
-  XSETFASTINT (limit1, (limit < endpos ? limit : endpos));
-  end = Fnext_single_property_change (position, propname, w->contents, limit1);
-  if (INTEGERP (end))
-    endpos = XINT (end);
 
   /* Look at properties from overlays.  */
   USE_SAFE_ALLOCA;
@@ -5913,12 +5898,8 @@ face_at_buffer_position (struct window *w, ptrdiff_t pos,
     ptrdiff_t next_overlay;
 
     GET_OVERLAYS_AT (pos, overlay_vec, noverlays, &next_overlay, false);
-    if (next_overlay < endpos)
-      endpos = next_overlay;
   }
 
-  *endptr = endpos;
-
   {
     int face_id;
 
@@ -5951,21 +5932,11 @@ face_at_buffer_position (struct window *w, ptrdiff_t pos,
   noverlays = sort_overlays (overlay_vec, noverlays, w);
   for (i = 0; i < noverlays; i++)
     {
-      Lisp_Object oend;
-      ptrdiff_t oendpos;
-
       prop = Foverlay_get (overlay_vec[i], propname);
       if (!NILP (prop))
 	merge_face_ref (f, prop, attrs, true, 0);
-
-      oend = OVERLAY_END (overlay_vec[i]);
-      oendpos = OVERLAY_POSITION (oend);
-      if (oendpos < endpos)
-	endpos = oendpos;
     }
 
-  *endptr = endpos;
-
   SAFE_FREE ();
 
   /* Look up a realized face with the given face attributes,
-- 
2.9.0

Re: Dead or unused face handling code

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>
> From: Dmitry Antipov <dmantipov@yandex.ru>
> Date: Thu, 7 Jul 2016 09:57:03 +0300
> 
> I've found a few dummy/unused bits around face_at_xxx functions, and running
> with these patches (which see) for a few days without any problems. But since
> redisplay is very tricky, I don't feel brave enough to install them without
> having some feedback first...

I'm sorry, but I don't want to make these changes, for a couple of
reasons:

 . Changes like these are similar to whitespace changes, so they are
   generally okay as part of a larger changeset that fixes a real
   problem or adds a feature.  But when done alone, they are just
   nuisance: they make forensics harder for no good reason.  They also
   risk introducing bugs, and we don't really have a test suite for
   the display engine to be sure we didn't screw up anything.

 . The functionality you are proposing to delete is useful, even if
   currently largely unused.  We are extremely short on people who can
   resurrect this code if it is ever needed in the future; I'm not
   sure how many people here even know how to begin writing such code.
   (Looking up deleted code in Git history is not trivial, many people
   simply don't know how to do that, and won't try in the first place,
   because they won't know such code ever existed.)  So by deleting
   these seemingly unneeded bits, we will forever lose non-trivial
   baggage that no one on board, except maybe yours truly, can
   restore.

Thanks.

Display test suite

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 1231 bytes --]

On 2016-07-07 11:15, Eli Zaretskii wrote:
> They also risk introducing bugs, and we don't really have a test
> suite for the display engine to be sure we didn't screw up anything.

Richard recently suggested giving Emacs the ability to take a screenshot of itself. Could we imagine using this to run regression tests of the display engine?

I already use a similar process in one of my packages: the idea is to script Emacs using a simple DSL (see https://github.com/cpitclaudel/company-coq/blob/master/etc/rebuild-screenshots.el#L419 for an example, and https://github.com/cpitclaudel/company-coq/blob/master/img/match-function.gif for the output), and record a screenshot after each command; then, I use these images to generate the README page on https://github.com/cpitclaudel/company-coq/. Since these images are checked in, any time a change has visible consequences I see it immediately, and I can use ImageMagick to get a visual diff.

Of course, it could be hard to get screeenshots to looks exactly the same for everyone. Instead, we could have a makefile target that builds a large collection of these screenshots, and another one that rebuilds it and compares against a previously generated one.

Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Display test suite

[Eli Zaretskii]

> From: Clément Pit--Claudel <clement.pit@gmail.com>
> Date: Thu, 7 Jul 2016 11:47:54 -0400
> 
> Richard recently suggested giving Emacs the ability to take a screenshot of itself. Could we imagine using this to run regression tests of the display engine?

Maybe I'm missing something, but I don't see how.  A test suite needs
to include the expected results for each test.  Even if you include an
image file for each test that shows what should be displayed, AFAIU
the image will be specific to the display resolution, number of
colors, dimensions, etc., so inappropriate for automatic comparison
with the screenshot on the target machine.  Am I right?

Also, comparing images that could have a small number of pixels
different in some corner is error-prone.

IMO, a test suite for the display engine needs a way to describe
precisely what's on the screen, in some language, and then compare
those descriptions.

> Of course, it could be hard to get screeenshots to looks exactly the same for everyone. Instead, we could have a makefile target that builds a large collection of these screenshots, and another one that rebuilds it and compares against a previously generated one.

But then each time I start using a new machine, my reference point is
reset to a newer date, and I lose the ability for comparing against
older versions.

Thanks.

Re: Display test suite

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 2549 bytes --]

On 2016-07-07 12:29, Eli Zaretskii wrote:
>> From: Clément Pit--Claudel <clement.pit@gmail.com> Date: Thu, 7 Jul
>> 2016 11:47:54 -0400
>> 
>> Richard recently suggested giving Emacs the ability to take a
>> screenshot of itself. Could we imagine using this to run regression
>> tests of the display engine?
> 
> Maybe I'm missing something, but I don't see how.  A test suite
> needs to include the expected results for each test.  Even if you
> include an image file for each test that shows what should be
> displayed, AFAIU the image will be specific to the display
> resolution, number of colors, dimensions, etc., so inappropriate for
> automatic comparison with the screenshot on the target machine.  Am I
> right?

You're right; that's why I can only run the company-coq tests on my machine. Still, one might imagine setting up Emacs in a particular way to make them reproducible. Along these lines, I run the PNG through an optimizer and strip the metadata to make sure that I get the same output every time.

> Also, comparing images that could have a small number of pixels 
> different in some corner is error-prone.

There are very nice tools to do image diffs; they will highlight the offending pixels in a different color.

> IMO, a test suite for the display engine needs a way to describe 
> precisely what's on the screen, in some language, and then compare 
> those descriptions.

Yes, that would be nice (we have something like that for font-lock, right?)

>> Of course, it could be hard to get screeenshots to looks exactly
>> the same for everyone. Instead, we could have a makefile target
>> that builds a large collection of these screenshots, and another
>> one that rebuilds it and compares against a previously generated
>> one.
> 
> But then each time I start using a new machine, my reference point
> is reset to a newer date, and I lose the ability for comparing
> against older versions.

Absolutely. I think such tests would serve as a nice way to check your own commits (for before/after tests).

Also, maybe the variability is a good point: each people regularly run these tests, then changes that affect only one configuration would be noticed. If we have tests that produce the same output for every one, changes that affect only one particular set of configurations would not be noticed.

In any case, I've found the graphical tests that I do for company-coq very useful; whether they could generalize to all of Emacs is a non-trivial question.

Cheers,
Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Display test suite

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Richard recently suggested giving Emacs the ability to take a
  > screenshot of itself. Could we imagine using this to run
  > regression tests of the display engine?

This makes me wonder if a Linux console has the ability to make
a textual screenshot as an array of characters.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: Display test suite

[Kaushal Modi]

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

If understand correctly, https://asciinema.org records screen casts using
that technique. The recordings are in form of .json files.

On Fri, Jul 8, 2016, 9:40 AM Richard Stallman <rms@gnu.org> wrote:

>
> This makes me wonder if a Linux console has the ability to make
> a textual screenshot as an array of characters.
>
-- 

-- 
Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 695 bytes --]

Re: Display test suite

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 228 bytes --]

On 2016-07-08 09:44, Kaushal Modi wrote:
> If understand correctly, https://asciinema.org records screen casts using that technique. The recordings are in form of .json files. 

Yup; they do this by emulating a TTY, IIRC.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Display test suite

[Andreas Schwab]

Richard Stallman <rms@gnu.org> writes:

> This makes me wonder if a Linux console has the ability to make
> a textual screenshot as an array of characters.

It does.

  7 char	Virtual console capture devices
		  0 = /dev/vcs		Current vc text contents
		  1 = /dev/vcs1		tty1 text contents
		    ...
		 63 = /dev/vcs63	tty63 text contents
		128 = /dev/vcsa		Current vc text/attribute contents
		129 = /dev/vcsa1	tty1 text/attribute contents
		    ...
		191 = /dev/vcsa63	tty63 text/attribute contents

		NOTE: These devices permit both read and write access.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Display test suite

[raman]

Richard Stallman <rms@gnu.org> writes:

Not sure if this ansers your question -- but 
setterm -dump  gives you a  text file of the console contents  -- with
newline chars separating lines.> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>
>   > Richard recently suggested giving Emacs the ability to take a
>   > screenshot of itself. Could we imagine using this to run
>   > regression tests of the display engine?
>
> This makes me wonder if a Linux console has the ability to make
> a textual screenshot as an array of characters.

-- 

Re: Dead or unused face handling code

[Dmitry Antipov]

On 07/07/2016 06:15 PM, Eli Zaretskii wrote:

> I'm sorry, but I don't want to make these changes, for a couple of
> reasons:

Although I do not agree with some of your points, OK just because
you're the king of that mountain.

Dmitry