[RFC PATCH] Per-window face support

[RFC PATCH] Per-window face support

[dancol]

This patch lets us actually vary faces on a window-by-window basis. It
works by allowing face-remapping-alist entries to apply only to windows
with specific parameters set. A package that wants to apply a face
remapping to all windows with certain characteristics should use
face-remapping to add a remapping in all buffers, then set window
parameters to make that remapping take effect some of the time.

The existing support for per-window overlays isn't sufficient, since 1)
overlays affect only buffer text, and so can't affect non-text
face-configured regions like the beyond EOB text area.

diff --git a/src/dispextern.h b/src/dispextern.h
index bc2a76f1e0..2180c9ae63 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3429,11 +3429,12 @@ char *choose_face_font (struct frame *,
Lisp_Object *, Lisp_Object,
 #ifdef HAVE_WINDOW_SYSTEM
 void prepare_face_for_display (struct frame *, struct face *);
 #endif
-int lookup_named_face (struct frame *, Lisp_Object, bool);
-int lookup_basic_face (struct frame *, int);
+int lookup_named_face (struct window *, struct frame *, Lisp_Object, bool);
+int lookup_basic_face (struct window *, struct frame *, int);
 int smaller_face (struct frame *, int, int);
 int face_with_height (struct frame *, int, int);
-int lookup_derived_face (struct frame *, Lisp_Object, int, bool);
+int lookup_derived_face (struct window *, struct frame *,
+                         Lisp_Object, int, bool);
 void init_frame_faces (struct frame *);
 void free_frame_faces (struct frame *);
 void recompute_basic_faces (struct frame *);
@@ -3443,7 +3444,7 @@ 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 merge_faces (struct frame *, Lisp_Object, int, int);
+int merge_faces (struct window *, Lisp_Object, int, int);
 int compute_char_face (struct frame *, int, Lisp_Object);
 void free_all_realized_faces (Lisp_Object);
 extern char unspecified_fg[], unspecified_bg[];
diff --git a/src/dispnew.c b/src/dispnew.c
index b854d179d5..46e0c83ef6 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -2508,8 +2508,7 @@ spec_glyph_lookup_face (struct window *w, GLYPH *glyph)
   /* Convert the glyph's specified face to a realized (cache) face.  */
   if (lface_id > 0)
     {
-      int face_id = merge_faces (XFRAME (w->frame),
-				 Qt, lface_id, DEFAULT_FACE_ID);
+      int face_id = merge_faces (w, Qt, lface_id, DEFAULT_FACE_ID);
       SET_GLYPH_FACE (*glyph, face_id);
     }
 }
diff --git a/src/font.c b/src/font.c
index 305bb14576..c886c299d1 100644
--- a/src/font.c
+++ b/src/font.c
@@ -3810,7 +3810,7 @@ font_range (ptrdiff_t pos, ptrdiff_t pos_byte,
ptrdiff_t *limit,
 	  face_id =
 	    NILP (Vface_remapping_alist)
 	    ? DEFAULT_FACE_ID
-	    : lookup_basic_face (f, DEFAULT_FACE_ID);
+	    : lookup_basic_face (w, f, DEFAULT_FACE_ID);

 	  face_id = face_at_string_position (w, string, pos, 0, &ignore,
 					     face_id, false);
@@ -4559,7 +4559,7 @@ DEFUN ("internal-char-font", Finternal_char_font,
Sinternal_char_font, 1, 2, 0,
       CHECK_CHARACTER (ch);
       c = XINT (ch);
       f = XFRAME (selected_frame);
-      face_id = lookup_basic_face (f, DEFAULT_FACE_ID);
+      face_id = lookup_basic_face (NULL, f, DEFAULT_FACE_ID);
       pos = -1;
     }
   else
diff --git a/src/fringe.c b/src/fringe.c
index 85aa14da72..6069184681 100644
--- a/src/fringe.c
+++ b/src/fringe.c
@@ -587,8 +587,8 @@ draw_fringe_bitmap_1 (struct window *w, struct
glyph_row *row, int left_p, int o
   if (face_id == DEFAULT_FACE_ID)
     {
       Lisp_Object face = fringe_faces[which];
-      face_id = NILP (face) ? lookup_named_face (f, Qfringe, false)
-	: lookup_derived_face (f, face, FRINGE_FACE_ID, 0);
+      face_id = NILP (face) ? lookup_named_face (w, f, Qfringe, false)
+	: lookup_derived_face (w, f, face, FRINGE_FACE_ID, 0);
       if (face_id < 0)
 	face_id = FRINGE_FACE_ID;
     }
@@ -1633,20 +1633,10 @@ If FACE is nil, reset face to default fringe face.
 */)
   if (!n)
     error ("Undefined fringe bitmap");

-  /* The purpose of the following code is to signal an error if FACE
-     is not a face.  This is for the caller's convenience only; the
-     redisplay code should be able to fail gracefully.  Skip the check
-     if FRINGE_FACE_ID is unrealized (as in batch mode and during
-     daemon startup).  */
-  if (!NILP (face))
-    {
-      struct frame *f = SELECTED_FRAME ();
-
-      if (FACE_FROM_ID_OR_NULL (f, FRINGE_FACE_ID)
-	  && lookup_derived_face (f, face, FRINGE_FACE_ID, 1) < 0)
-	error ("No such face");
-    }
-
+  /* We used to check, as a convenience to callers, for basic face
+     validity here, but since validity can depend on the specific
+     _window_ in which this buffer is being displayed, defer the check
+     to redisplay, which can cope with bad face specifications.  */
   fringe_faces[n] = face;
   return Qnil;
 }
diff --git a/src/msdos.c b/src/msdos.c
index eedbf7b1a6..6c0dfa0c46 100644
--- a/src/msdos.c
+++ b/src/msdos.c
@@ -3063,15 +3063,15 @@ XMenuActivate (Display *foo, XMenu *menu, int
*pane, int *selidx,
   state = alloca (menu->panecount * sizeof (struct IT_menu_state));
   screensize = screen_size * 2;
   faces[0]
-    = lookup_derived_face (sf, intern ("msdos-menu-passive-face"),
+    = lookup_derived_face (NULL, sf, intern ("msdos-menu-passive-face"),
 			   DEFAULT_FACE_ID, 1);
   faces[1]
-    = lookup_derived_face (sf, intern ("msdos-menu-active-face"),
+    = lookup_derived_face (NULL, sf, intern ("msdos-menu-active-face"),
 			   DEFAULT_FACE_ID, 1);
   selectface = intern ("msdos-menu-select-face");
-  faces[2] = lookup_derived_face (sf, selectface,
+  faces[2] = lookup_derived_face (NULL, sf, selectface,
 				  faces[0], 1);
-  faces[3] = lookup_derived_face (sf, selectface,
+  faces[3] = lookup_derived_face (NULL, sf, selectface,
 				  faces[1], 1);

   /* Make sure the menu title is always displayed with
diff --git a/src/term.c b/src/term.c
index 08d483f4fa..bcd7dd82d6 100644
--- a/src/term.c
+++ b/src/term.c
@@ -3132,15 +3132,15 @@ tty_menu_activate (tty_menu *menu, int *pane, int
*selidx,
   SAFE_NALLOCA (state, 1, menu->panecount);
   memset (state, 0, sizeof (*state));
   faces[0]
-    = lookup_derived_face (sf, intern ("tty-menu-disabled-face"),
+    = lookup_derived_face (NULL, sf, intern ("tty-menu-disabled-face"),
 			   DEFAULT_FACE_ID, 1);
   faces[1]
-    = lookup_derived_face (sf, intern ("tty-menu-enabled-face"),
+    = lookup_derived_face (NULL, sf, intern ("tty-menu-enabled-face"),
 			   DEFAULT_FACE_ID, 1);
   selectface = intern ("tty-menu-selected-face");
-  faces[2] = lookup_derived_face (sf, selectface,
+  faces[2] = lookup_derived_face (NULL, sf, selectface,
 				  faces[0], 1);
-  faces[3] = lookup_derived_face (sf, selectface,
+  faces[3] = lookup_derived_face (NULL, sf, selectface,
 				  faces[1], 1);

   /* Make sure the menu title is always displayed with
diff --git a/src/xdisp.c b/src/xdisp.c
index 1299ba38e3..594a41f401 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -2809,7 +2809,7 @@ init_iterator (struct it *it, struct window *w,
   /* Perhaps remap BASE_FACE_ID to a user-specified alternative.  */
   if (! NILP (Vface_remapping_alist))
     remapped_base_face_id
-      = lookup_basic_face (XFRAME (w->frame), base_face_id);
+      = lookup_basic_face (w, XFRAME (w->frame), base_face_id);

   /* Use one of the mode line rows of W's desired matrix if
      appropriate.  */
@@ -4060,7 +4060,7 @@ handle_face_prop (struct it *it)
 	     might be a big deal.  */
 	  base_face_id = it->string_from_prefix_prop_p
 	    ? (!NILP (Vface_remapping_alist)
-	       ? lookup_basic_face (it->f, DEFAULT_FACE_ID)
+	       ? lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID)
 	       : DEFAULT_FACE_ID)
 	    : underlying_face_id (it);
 	}
@@ -4988,7 +4988,7 @@ handle_single_display_spec (struct it *it,
Lisp_Object spec, Lisp_Object object,
 		  struct face *f;

 		  f = FACE_FROM_ID (it->f,
-				    lookup_basic_face (it->f, DEFAULT_FACE_ID));
+				    lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID));
 		  new_height = (XFLOATINT (it->font_height)
 				* XINT (f->lface[LFACE_HEIGHT_INDEX]));
 		}
@@ -5175,12 +5175,12 @@ handle_single_display_spec (struct it *it,
Lisp_Object spec, Lisp_Object object,

       if (it)
 	{
-	  int face_id = lookup_basic_face (it->f, DEFAULT_FACE_ID);
+	  int face_id = lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID);

 	  if (CONSP (XCDR (XCDR (spec))))
 	    {
 	      Lisp_Object face_name = XCAR (XCDR (XCDR (spec)));
-	      int face_id2 = lookup_derived_face (it->f, face_name,
+	      int face_id2 = lookup_derived_face (it->w, it->f, face_name,
 						  FRINGE_FACE_ID, false);
 	      if (face_id2 >= 0)
 		face_id = face_id2;
@@ -6985,7 +6985,7 @@ merge_escape_glyph_face (struct it *it)
   else
     {
       /* Merge the `escape-glyph' face into the current face.  */
-      face_id = merge_faces (it->f, Qescape_glyph, 0, it->face_id);
+      face_id = merge_faces (it->w, Qescape_glyph, 0, it->face_id);
       last_escape_glyph_frame = it->f;
       last_escape_glyph_face_id = it->face_id;
       last_escape_glyph_merged_face_id = face_id;
@@ -7010,7 +7010,7 @@ merge_glyphless_glyph_face (struct it *it)
   else
     {
       /* Merge the `glyphless-char' face into the current face.  */
-      face_id = merge_faces (it->f, Qglyphless_char, 0, it->face_id);
+      face_id = merge_faces (it->w, Qglyphless_char, 0, it->face_id);
       last_glyphless_glyph_frame = it->f;
       last_glyphless_glyph_face_id = it->face_id;
       last_glyphless_glyph_merged_face_id = face_id;
@@ -7184,7 +7184,7 @@ get_next_display_element (struct it *it)
 		    }

 		  face_id = (lface_id
-			     ? merge_faces (it->f, Qt, lface_id, it->face_id)
+			     ? merge_faces (it->w, Qt, lface_id, it->face_id)
 			     : merge_escape_glyph_face (it));

 		  XSETINT (it->ctl_chars[0], g);
@@ -7199,7 +7199,7 @@ get_next_display_element (struct it *it)
 	      if (nonascii_space_p && EQ (Vnobreak_char_display, Qt))
 		{
 		  /* Merge `nobreak-space' into the current face.  */
-		  face_id = merge_faces (it->f, Qnobreak_space, 0,
+		  face_id = merge_faces (it->w, Qnobreak_space, 0,
 					 it->face_id);
 		  XSETINT (it->ctl_chars[0], ' ');
 		  ctl_len = 1;
@@ -7212,7 +7212,7 @@ get_next_display_element (struct it *it)
 	      if (nonascii_hyphen_p && EQ (Vnobreak_char_display, Qt))
 		{
 		  /* Merge `nobreak-space' into the current face.  */
-		  face_id = merge_faces (it->f, Qnobreak_hyphen, 0,
+		  face_id = merge_faces (it->w, Qnobreak_hyphen, 0,
 					 it->face_id);
 		  XSETINT (it->ctl_chars[0], '-');
 		  ctl_len = 1;
@@ -7232,7 +7232,7 @@ get_next_display_element (struct it *it)
 		}

 	      face_id = (lface_id
-			 ? merge_faces (it->f, Qt, lface_id, it->face_id)
+			 ? merge_faces (it->w, Qt, lface_id, it->face_id)
 			 : merge_escape_glyph_face (it));

 	      /* Draw non-ASCII space/hyphen with escape glyph: */
@@ -7860,7 +7860,7 @@ next_element_from_display_vector (struct it *it)
 	{
 	  int lface_id = GLYPH_CODE_FACE (gc);
 	  if (lface_id > 0)
-	    it->face_id = merge_faces (it->f, Qt, lface_id,
+	    it->face_id = merge_faces (it->w, Qt, lface_id,
 				       it->saved_face_id);
 	}

@@ -7889,7 +7889,7 @@ next_element_from_display_vector (struct it *it)
 		GLYPH_CODE_FACE (it->dpvec[it->current.dpvec_index + 1]);

 	      if (lface_id > 0)
-		next_face_id = merge_faces (it->f, Qt, lface_id,
+		next_face_id = merge_faces (it->w, Qt, lface_id,
 					    it->saved_face_id);
 	    }
 	}
@@ -20084,7 +20084,7 @@ append_space_for_newline (struct it *it, bool
default_face_p)
 	  /* If the default face was remapped, be sure to use the
 	     remapped face for the appended newline.  */
 	  if (default_face_p)
-	    it->face_id = lookup_basic_face (it->f, DEFAULT_FACE_ID);
+	    it->face_id = lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID);
 	  else if (it->face_before_selective_p)
 	    it->face_id = it->saved_face_id;
 	  face = FACE_FROM_ID (it->f, it->face_id);
@@ -20231,8 +20231,9 @@ extend_face_to_end_of_line (struct it *it)
     return;

   /* The default face, possibly remapped. */
-  default_face = FACE_FROM_ID_OR_NULL (f,
-				       lookup_basic_face (f, DEFAULT_FACE_ID));
+  default_face = FACE_FROM_ID_OR_NULL (
+    f,
+    lookup_basic_face (it->w, f, DEFAULT_FACE_ID));

   /* Face extension extends the background and box of IT->face_id
      to the end of the line.  If the background equals the background
@@ -20486,11 +20487,12 @@ trailing_whitespace_p (ptrdiff_t charpos)
 }


-/* Highlight trailing whitespace, if any, in ROW.  */
+/* Highlight trailing whitespace, if any, in row at IT.  */

 static void
-highlight_trailing_whitespace (struct frame *f, struct glyph_row *row)
+highlight_trailing_whitespace (struct it *it)
 {
+  struct glyph_row *row = it->glyph_row;
   int used = row->used[TEXT_AREA];

   if (used)
@@ -20535,7 +20537,7 @@ highlight_trailing_whitespace (struct frame *f,
struct glyph_row *row)
 		  && glyph->u.ch == ' '))
 	  && trailing_whitespace_p (glyph->charpos))
 	{
-	  int face_id = lookup_named_face (f, Qtrailing_whitespace, false);
+	  int face_id = lookup_named_face (it->w, it->f, Qtrailing_whitespace,
false);
 	  if (face_id < 0)
 	    return;

@@ -21107,9 +21109,9 @@ maybe_produce_line_number (struct it *it)
   char lnum_buf[INT_STRLEN_BOUND (ptrdiff_t) + 1];
   bool beyond_zv = IT_BYTEPOS (*it) >= ZV_BYTE ? true : false;
   ptrdiff_t lnum_offset = -1; /* to produce 1-based line numbers */
-  int lnum_face_id = merge_faces (it->f, Qline_number, 0, DEFAULT_FACE_ID);
+  int lnum_face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID);
   int current_lnum_face_id
-    = merge_faces (it->f, Qline_number_current_line, 0, DEFAULT_FACE_ID);
+    = merge_faces (it->w, Qline_number_current_line, 0, DEFAULT_FACE_ID);
   /* Compute point's line number if needed.  */
   if ((EQ (Vdisplay_line_numbers, Qrelative)
        || EQ (Vdisplay_line_numbers, Qvisual)
@@ -21559,7 +21561,8 @@ display_line (struct it *it, int cursor_vpos)
 	     portions of the screen will clear with the default face's
 	     background color.  */
 	  if (row->reversed_p
-	      || lookup_basic_face (it->f, DEFAULT_FACE_ID) != DEFAULT_FACE_ID)
+	      || lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID)
+              != DEFAULT_FACE_ID)
 	    extend_face_to_end_of_line (it);
 	  break;
 	}
@@ -22192,7 +22195,7 @@ display_line (struct it *it, int cursor_vpos)

   /* Highlight trailing whitespace.  */
   if (!NILP (Vshow_trailing_whitespace))
-    highlight_trailing_whitespace (it->f, it->glyph_row);
+    highlight_trailing_whitespace (it);

   /* Compute pixel dimensions of this line.  */
   compute_line_metrics (it);
@@ -27862,7 +27865,7 @@ calc_line_height_property (struct it *it,
Lisp_Object val, struct font *font,
       int face_id;
       struct face *face;

-      face_id = lookup_named_face (it->f, face_name, false);
+      face_id = lookup_named_face (it->w, it->f, face_name, false);
       face = FACE_FROM_ID_OR_NULL (it->f, face_id);
       if (face == NULL || ((font = face->font) == NULL))
 	return make_number (-1);
diff --git a/src/xfaces.c b/src/xfaces.c
index a9c2f37e9f..bf0b1ea04f 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -350,7 +350,8 @@ static bool realize_default_face (struct frame *);
 static void realize_named_face (struct frame *, Lisp_Object, int);
 static struct face_cache *make_face_cache (struct frame *);
 static void free_face_cache (struct face_cache *);
-static bool merge_face_ref (struct frame *, Lisp_Object, Lisp_Object *,
+static bool merge_face_ref (struct window *w,
+                            struct frame *, Lisp_Object, Lisp_Object *,
 			    bool, struct named_merge_point *);
 static int color_distance (XColor *x, XColor *y);

@@ -1551,7 +1552,7 @@ the WIDTH times as wide as FACE on FRAME.  */)
     {
       /* This is of limited utility since it works with character
 	 widths.  Keep it for compatibility.  --gerd.  */
-      int face_id = lookup_named_face (f, face, false);
+      int face_id = lookup_named_face (NULL, f, face, false);
       struct face *width_face = FACE_FROM_ID_OR_NULL (f, face_id);

       if (width_face && width_face->font)
@@ -1907,19 +1908,22 @@ get_lface_attributes_no_remap (struct frame *f,
Lisp_Object face_name,
   return !NILP (lface);
 }

-/* Get face attributes of face FACE_NAME from frame-local faces on frame
-   F.  Store the resulting attributes in ATTRS which must point to a
-   vector of Lisp_Objects of size LFACE_VECTOR_SIZE.  If FACE_NAME is an
-   alias for another face, use that face's definition.
-   If SIGNAL_P, signal an error if FACE_NAME does not name a face.
-   Otherwise, return true iff FACE_NAME is a face.  */
-
+/* Get face attributes of face FACE_NAME from frame-local faces on
+   frame F.  Store the resulting attributes in ATTRS which must point
+   to a vector of Lisp_Objects of size LFACE_VECTOR_SIZE.
+   If FACE_NAME is an alias for another face, use that face's
+   definition.  If SIGNAL_P, signal an error if FACE_NAME does not
+   name a face.  Otherwise, return true iff FACE_NAME is a face.  If W
+   is non-NULL, also consider remappings attached to the window.
+   */
 static bool
-get_lface_attributes (struct frame *f, Lisp_Object face_name,
+get_lface_attributes (struct window *w,
+                      struct frame *f, Lisp_Object face_name,
 		      Lisp_Object attrs[LFACE_VECTOR_SIZE], bool signal_p,
 		      struct named_merge_point *named_merge_points)
 {
   Lisp_Object face_remapping;
+  eassert (w == NULL || WINDOW_XFRAME (w) == f);

   face_name = resolve_face_name (face_name, signal_p);

@@ -1939,7 +1943,7 @@ get_lface_attributes (struct frame *f, Lisp_Object
face_name,
 	  for (i = 1; i < LFACE_VECTOR_SIZE; ++i)
 	    attrs[i] = Qunspecified;

-	  return merge_face_ref (f, XCDR (face_remapping), attrs,
+	  return merge_face_ref (w, f, XCDR (face_remapping), attrs,
 				 signal_p, named_merge_points);
 	}
     }
@@ -2072,15 +2076,16 @@ merge_face_heights (Lisp_Object from, Lisp_Object
to, Lisp_Object invalid)

 /* Merge two Lisp face attribute vectors on frame F, FROM and TO, and
    store the resulting attributes in TO, which must be already be
-   completely specified and contain only absolute attributes.  Every
-   specified attribute of FROM overrides the corresponding attribute of
-   TO; relative attributes in FROM are merged with the absolute value in
-   TO and replace it.  NAMED_MERGE_POINTS is used internally to detect
-   loops in face inheritance/remapping; it should be 0 when called from
-   other places.  */
-
+   completely specified and contain only absolute attributes.
+   Every specified attribute of FROM overrides the corresponding
+   attribute of TO; relative attributes in FROM are merged with the
+   absolute value in TO and replace it.  NAMED_MERGE_POINTS is used
+   internally to detect loops in face inheritance/remapping; it should
+   be 0 when called from other places.  If window W is non-NULL, use W
+   to interpret face specifications. */
 static void
-merge_face_vectors (struct frame *f, Lisp_Object *from, Lisp_Object *to,
+merge_face_vectors (struct window *w,
+                    struct frame *f, Lisp_Object *from, Lisp_Object *to,
 		    struct named_merge_point *named_merge_points)
 {
   int i;
@@ -2093,7 +2098,8 @@ merge_face_vectors (struct frame *f, Lisp_Object
*from, Lisp_Object *to,
      other code uses `unspecified' as a generic value for face
attributes. */
   if (!UNSPECIFIEDP (from[LFACE_INHERIT_INDEX])
       && !NILP (from[LFACE_INHERIT_INDEX]))
-    merge_face_ref (f, from[LFACE_INHERIT_INDEX], to, false,
named_merge_points);
+    merge_face_ref (w, f, from[LFACE_INHERIT_INDEX],
+                    to, false, named_merge_points);

   if (FONT_SPEC_P (from[LFACE_FONT_INDEX]))
     {
@@ -2153,10 +2159,12 @@ merge_face_vectors (struct frame *f, Lisp_Object
*from, Lisp_Object *to,
 /* Merge the named face FACE_NAME on frame F, into the vector of face
    attributes TO.  Use NAMED_MERGE_POINTS to detect loops in face
    inheritance.  Return true if FACE_NAME is a valid face name and
-   merging succeeded.  */
+   merging succeeded.  Window W, if non-NULL, is used to filter face
+   specifications. */

 static bool
-merge_named_face (struct frame *f, Lisp_Object face_name, Lisp_Object *to,
+merge_named_face (struct window *w,
+                  struct frame *f, Lisp_Object face_name, Lisp_Object *to,
 		  struct named_merge_point *named_merge_points)
 {
   struct named_merge_point named_merge_point;
@@ -2166,11 +2174,11 @@ merge_named_face (struct frame *f, Lisp_Object
face_name, Lisp_Object *to,
 			      &named_merge_points))
     {
       Lisp_Object from[LFACE_VECTOR_SIZE];
-      bool ok = get_lface_attributes (f, face_name, from, false,
+      bool ok = get_lface_attributes (w, f, face_name, from, false,
 				      named_merge_points);

       if (ok)
-	merge_face_vectors (f, from, to, named_merge_points);
+	merge_face_vectors (w, f, from, to, named_merge_points);

       return ok;
     }
@@ -2178,6 +2186,111 @@ merge_named_face (struct frame *f, Lisp_Object
face_name, Lisp_Object *to,
     return false;
 }

+/* Determine whether the face filter FILTER evaluated in window W
+   matches.  W can be NULL if the window context is unknown.
+
+   A face filter is either nil, which always matches, or a list
+   (:window PARAMETER VALUE), which matches if the current window has
+   a PARAMETER EQ to VALUE.
+
+   If the filter is invalid, set *OK to false and, if ERR_MSGS is
+   true, log an error message.  */
+static bool
+evaluate_face_filter (Lisp_Object filter, struct window *w,
+                      bool *ok, bool err_msgs)
+{
+  Lisp_Object orig_filter = filter;
+
+  {
+    if (NILP (filter))
+      return true;
+
+    if (face_filters_always_match)
+      return true;
+
+    if (!CONSP (filter))
+      goto err;
+
+    if (!EQ (XCAR (filter), Qwindow_kw))
+      goto err;
+    filter = XCDR (filter);
+
+    Lisp_Object parameter = XCAR (filter);
+    filter = XCDR (filter);
+    if (!CONSP (filter))
+      goto err;
+
+    Lisp_Object value = XCAR (filter);
+    filter = XCDR (filter);
+    if (!NILP (filter))
+      goto err;
+
+    bool match = false;
+    if (w) {
+      Lisp_Object found = assq_no_quit (parameter, w->window_parameters);
+      if (!NILP (found) && EQ (XCDR (found), value))
+        match = true;
+    }
+
+    return match;
+  }
+
+ err:
+  if (err_msgs)
+    add_to_log ("Invalid face filter %S", orig_filter);
+  *ok = false;
+  return false;
+}
+
+/* Determine whether FACE_REF is a "filter" face specification (case
+   #4 in merge_face_ref). If it is, evaluate the filter, and if the
+   filter matches, return the filtered expression. Otherwise, return
+   the original expression.
+
+   On error, set *OK to false, having logged an error message if
+   ERR_MSGS is true, with return value unspecified.
+
+   W is either NULL or a window used to evaluate filters. If W is
+   null, no window-based face specification filter matches.
+*/
+static Lisp_Object
+filter_face_ref (Lisp_Object face_ref,
+                 struct window *w,
+                 bool *ok,
+                 bool err_msgs)
+{
+  Lisp_Object orig_face_ref = face_ref;
+  if (!CONSP (face_ref))
+    return face_ref;
+
+  {
+    if (!EQ (XCAR (face_ref), Qfiltered_kw))
+      return face_ref;
+    face_ref = XCDR (face_ref);
+
+    if (!CONSP (face_ref))
+      goto err;
+    Lisp_Object filter = XCAR (face_ref);
+    face_ref = XCDR (face_ref);
+
+    if (!CONSP (face_ref))
+      goto err;
+    Lisp_Object filtered_face_ref = XCAR (face_ref);
+    face_ref = XCDR (face_ref);
+
+    if (!NILP (face_ref))
+      goto err;
+
+    return evaluate_face_filter (filter, w, ok, err_msgs)
+      ? filtered_face_ref : Qnil;
+  }
+
+ err:
+  if (err_msgs)
+    add_to_log ("Invalid face ref %S", orig_face_ref);
+  *ok = false;
+  return Qnil;
+}

 /* Merge face attributes from the lisp `face reference' FACE_REF on
    frame F into the face attribute vector TO.  If ERR_MSGS,
@@ -2199,21 +2312,44 @@ merge_named_face (struct frame *f, Lisp_Object
face_name, Lisp_Object *to,
    (BACKGROUND-COLOR . COLOR) where COLOR is a color name.  This is
    for compatibility with 20.2.

+   4. Conses of the form
+   (:filter (:window PARAMETER VALUE) FACE-SPECIFICATION),
+   which applies FACE-SPECIFICATION only if the
+   given face attributes are being evaluated in the context of a
+   window with a parameter named PARAMETER being EQ VALUE.
+
+   5. nil, which means to merge nothing.
+
    Face specifications earlier in lists take precedence over later
    specifications.  */

 static bool
-merge_face_ref (struct frame *f, Lisp_Object face_ref, Lisp_Object *to,
+merge_face_ref (struct window *w,
+                struct frame *f, Lisp_Object face_ref, Lisp_Object *to,
 		bool err_msgs, struct named_merge_point *named_merge_points)
 {
   bool ok = true;		/* Succeed without an error? */
+  Lisp_Object filtered_face_ref;
+
+  filtered_face_ref = face_ref;
+  do
+    {
+      face_ref = filtered_face_ref;
+      filtered_face_ref = filter_face_ref (face_ref, w, &ok, err_msgs);
+    } while (ok && !EQ (face_ref, filtered_face_ref));
+
+  if (!ok)
+    return false;
+
+  if (NILP (face_ref))
+    return true;

   if (CONSP (face_ref))
     {
       Lisp_Object first = XCAR (face_ref);

       if (EQ (first, Qforeground_color)
-	  || EQ (first, Qbackground_color))
+               || EQ (first, Qbackground_color))
 	{
 	  /* One of (FOREGROUND-COLOR . COLOR) or (BACKGROUND-COLOR
 	     . COLOR).  COLOR must be a string.  */
@@ -2400,7 +2536,7 @@ merge_face_ref (struct frame *f, Lisp_Object
face_ref, Lisp_Object *to,
 		{
 		  /* This is not really very useful; it's just like a
 		     normal face reference.  */
-		  if (! merge_face_ref (f, value, to,
+		  if (! merge_face_ref (w, f, value, to,
 					err_msgs, named_merge_points))
 		    err = true;
 		}
@@ -2424,16 +2560,16 @@ merge_face_ref (struct frame *f, Lisp_Object
face_ref, Lisp_Object *to,
 	  Lisp_Object next = XCDR (face_ref);

 	  if (! NILP (next))
-	    ok = merge_face_ref (f, next, to, err_msgs, named_merge_points);
+	    ok = merge_face_ref (w, f, next, to, err_msgs, named_merge_points);

-	  if (! merge_face_ref (f, first, to, err_msgs, named_merge_points))
+	  if (! merge_face_ref (w, f, first, to, err_msgs, named_merge_points))
 	    ok = false;
 	}
     }
   else
     {
       /* FACE_REF ought to be a face name.  */
-      ok = merge_named_face (f, face_ref, to, named_merge_points);
+      ok = merge_named_face (w, f, face_ref, to, named_merge_points);
       if (!ok && err_msgs)
 	add_to_log ("Invalid face reference: %s", face_ref);
     }
@@ -3701,7 +3837,7 @@ Default face attributes override any local face
attributes.  */)
 	  /* Ensure that the face vector is fully specified by merging
 	     the previously-cached vector.  */
 	  memcpy (attrs, oldface->lface, sizeof attrs);
-	  merge_face_vectors (f, lvec, attrs, 0);
+	  merge_face_vectors (NULL, f, lvec, attrs, 0);
 	  vcopy (local_lface, 0, attrs, LFACE_VECTOR_SIZE);
 	  newface = realize_face (c, lvec, DEFAULT_FACE_ID);

@@ -3774,7 +3910,7 @@ return the font name used for CHARACTER.  */)
   else
     {
       struct frame *f = decode_live_frame (frame);
-      int face_id = lookup_named_face (f, face, true);
+      int face_id = lookup_named_face (NULL, f, face, true);
       struct face *fface = FACE_FROM_ID_OR_NULL (f, face_id);

       if (! fface)
@@ -4432,10 +4568,12 @@ face_for_font (struct frame *f, Lisp_Object
font_object, struct face *base_face)
 /* Return the face id of the realized face for named face SYMBOL on
    frame F suitable for displaying ASCII characters.  Value is -1 if
    the face couldn't be determined, which might happen if the default
-   face isn't realized and cannot be realized.  */
-
+   face isn't realized and cannot be realized.  If window W is given,
+   consider face remappings specified for W or for W's buffer. If W is
+   NULL, consider only frame-level face configuration.  */
 int
-lookup_named_face (struct frame *f, Lisp_Object symbol, bool signal_p)
+lookup_named_face (struct window *w, struct frame *f,
+                   Lisp_Object symbol, bool signal_p)
 {
   Lisp_Object attrs[LFACE_VECTOR_SIZE];
   Lisp_Object symbol_attrs[LFACE_VECTOR_SIZE];
@@ -4448,11 +4586,11 @@ lookup_named_face (struct frame *f, Lisp_Object
symbol, bool signal_p)
       default_face = FACE_FROM_ID (f, DEFAULT_FACE_ID);
     }

-  if (! get_lface_attributes (f, symbol, symbol_attrs, signal_p, 0))
+  if (! get_lface_attributes (w, f, symbol, symbol_attrs, signal_p, 0))
     return -1;

   memcpy (attrs, default_face->lface, sizeof attrs);
-  merge_face_vectors (f, symbol_attrs, attrs, 0);
+  merge_face_vectors (w, f, symbol_attrs, attrs, 0);

   return lookup_face (f, attrs);
 }
@@ -4462,10 +4600,10 @@ lookup_named_face (struct frame *f, Lisp_Object
symbol, bool signal_p)
    is FACE_ID.  The return value will usually simply be FACE_ID, unless that
    basic face has bee remapped via Vface_remapping_alist.  This function is
    conservative: if something goes wrong, it will simply return FACE_ID
-   rather than signal an error.   */
-
+   rather than signal an error.  Window W, if non-NULL, is used to filter
+   face specifications for remapping.  */
 int
-lookup_basic_face (struct frame *f, int face_id)
+lookup_basic_face (struct window *w, struct frame *f, int face_id)
 {
   Lisp_Object name, mapping;
   int remapped_face_id;
@@ -4505,7 +4643,7 @@ lookup_basic_face (struct frame *f, int face_id)

   /* If there is a remapping entry, lookup the face using NAME, which will
      handle the remapping too.  */
-  remapped_face_id = lookup_named_face (f, name, false);
+  remapped_face_id = lookup_named_face (w, f, name, false);
   if (remapped_face_id < 0)
     return face_id;		/* Give up. */

@@ -4603,22 +4741,23 @@ face_with_height (struct frame *f, int face_id,
int height)
    attributes of the face FACE_ID for attributes that aren't
    completely specified by SYMBOL.  This is like lookup_named_face,
    except that the default attributes come from FACE_ID, not from the
-   default face.  FACE_ID is assumed to be already realized.  */
-
+   default face.  FACE_ID is assumed to be already realized.
+   Window W, if non-NULL, filters face specifications.  */
 int
-lookup_derived_face (struct frame *f, Lisp_Object symbol, int face_id,
+lookup_derived_face (struct window *w,
+                     struct frame *f, Lisp_Object symbol, int face_id,
 		     bool signal_p)
 {
   Lisp_Object attrs[LFACE_VECTOR_SIZE];
   Lisp_Object symbol_attrs[LFACE_VECTOR_SIZE];
   struct face *default_face;

-  if (!get_lface_attributes (f, symbol, symbol_attrs, signal_p, 0))
+  if (!get_lface_attributes (w, f, symbol, symbol_attrs, signal_p, 0))
     return -1;

   default_face = FACE_FROM_ID (f, face_id);
   memcpy (attrs, default_face->lface, sizeof attrs);
-  merge_face_vectors (f, symbol_attrs, attrs, 0);
+  merge_face_vectors (w, f, symbol_attrs, attrs, 0);
   return lookup_face (f, attrs);
 }

@@ -4630,7 +4769,8 @@ DEFUN ("face-attributes-as-vector",
Fface_attributes_as_vector,
   Lisp_Object lface;
   lface = Fmake_vector (make_number (LFACE_VECTOR_SIZE),
 			Qunspecified);
-  merge_face_ref (XFRAME (selected_frame), plist, XVECTOR (lface)->contents,
+  merge_face_ref (NULL, XFRAME (selected_frame),
+                  plist, XVECTOR (lface)->contents,
 		  true, 0);
   return lface;
 }
@@ -4714,7 +4854,7 @@ x_supports_face_attributes_p (struct frame *f,

       memcpy (merged_attrs, def_attrs, sizeof merged_attrs);

-      merge_face_vectors (f, attrs, merged_attrs, 0);
+      merge_face_vectors (NULL, f, attrs, merged_attrs, 0);

       face_id = lookup_face (f, merged_attrs);
       face = FACE_FROM_ID_OR_NULL (f, face_id);
@@ -4985,7 +5125,7 @@ face for italic.  */)

   for (i = 0; i < LFACE_VECTOR_SIZE; i++)
     attrs[i] = Qunspecified;
-  merge_face_ref (f, attributes, attrs, true, 0);
+  merge_face_ref (NULL, f, attributes, attrs, true, 0);

   def_face = FACE_FROM_ID_OR_NULL (f, DEFAULT_FACE_ID);
   if (def_face == NULL)
@@ -5354,7 +5494,7 @@ realize_named_face (struct frame *f, Lisp_Object
symbol, int id)

   /* Merge SYMBOL's face with the default face.  */
   get_lface_attributes_no_remap (f, symbol, symbol_attrs, true);
-  merge_face_vectors (f, symbol_attrs, attrs, 0);
+  merge_face_vectors (NULL, f, symbol_attrs, attrs, 0);

   /* Realize the face.  */
   realize_face (c, attrs, id);
@@ -5869,7 +6009,7 @@ compute_char_face (struct frame *f, int ch,
Lisp_Object prop)
       Lisp_Object attrs[LFACE_VECTOR_SIZE];
       struct face *default_face = FACE_FROM_ID (f, DEFAULT_FACE_ID);
       memcpy (attrs, default_face->lface, sizeof attrs);
-      merge_face_ref (f, prop, attrs, true, 0);
+      merge_face_ref (NULL, f, prop, attrs, true, 0);
       face_id = lookup_face (f, attrs);
     }

@@ -5948,7 +6088,7 @@ face_at_buffer_position (struct window *w, ptrdiff_t
pos,
     else if (NILP (Vface_remapping_alist))
       face_id = DEFAULT_FACE_ID;
     else
-      face_id = lookup_basic_face (f, DEFAULT_FACE_ID);
+      face_id = lookup_basic_face (w, f, DEFAULT_FACE_ID);

     default_face = FACE_FROM_ID (f, face_id);
   }
@@ -5966,7 +6106,7 @@ face_at_buffer_position (struct window *w, ptrdiff_t
pos,

   /* Merge in attributes specified via text properties.  */
   if (!NILP (prop))
-    merge_face_ref (f, prop, attrs, true, 0);
+    merge_face_ref (w, f, prop, attrs, true, 0);

   /* Now merge the overlay data.  */
   noverlays = sort_overlays (overlay_vec, noverlays, w);
@@ -5986,7 +6126,7 @@ face_at_buffer_position (struct window *w, ptrdiff_t
pos,
 		 so discard the mouse-face text property, if any, and
 		 use the overlay property instead.  */
 	      memcpy (attrs, default_face->lface, sizeof attrs);
-	      merge_face_ref (f, prop, attrs, true, 0);
+	      merge_face_ref (w, f, prop, attrs, true, 0);
 	    }

 	  oend = OVERLAY_END (overlay_vec[i]);
@@ -6004,7 +6144,7 @@ face_at_buffer_position (struct window *w, ptrdiff_t
pos,

 	  prop = Foverlay_get (overlay_vec[i], propname);
 	  if (!NILP (prop))
-	    merge_face_ref (f, prop, attrs, true, 0);
+	    merge_face_ref (w, f, prop, attrs, true, 0);

 	  oend = OVERLAY_END (overlay_vec[i]);
 	  oendpos = OVERLAY_POSITION (oend);
@@ -6065,12 +6205,12 @@ face_for_overlay_string (struct window *w,
ptrdiff_t pos,
     return DEFAULT_FACE_ID;

   /* Begin with attributes from the default face.  */
-  default_face = FACE_FROM_ID (f, lookup_basic_face (f, DEFAULT_FACE_ID));
+  default_face = FACE_FROM_ID (f, lookup_basic_face (w, f,
DEFAULT_FACE_ID));
   memcpy (attrs, default_face->lface, sizeof attrs);

   /* Merge in attributes specified via text properties.  */
   if (!NILP (prop))
-    merge_face_ref (f, prop, attrs, true, 0);
+    merge_face_ref (w, f, prop, attrs, true, 0);

   *endptr = endpos;

@@ -6149,7 +6289,7 @@ face_at_string_position (struct window *w,
Lisp_Object string,

   /* Merge in attributes specified via text properties.  */
   if (!NILP (prop))
-    merge_face_ref (f, prop, attrs, true, 0);
+    merge_face_ref (w, f, prop, attrs, true, 0);

   /* Look up a realized face with the given face attributes,
      or realize a new one for ASCII characters.  */
@@ -6159,7 +6299,7 @@ face_at_string_position (struct window *w,
Lisp_Object string,

 /* Merge a face into a realized face.

-   F is frame where faces are (to be) realized.
+   W is a window in the frame where faces are (to be) realized.

    FACE_NAME is named face to merge.

@@ -6173,9 +6313,10 @@ face_at_string_position (struct window *w,
Lisp_Object string,
 */

 int
-merge_faces (struct frame *f, Lisp_Object face_name, int face_id,
+merge_faces (struct window *w, Lisp_Object face_name, int face_id,
 	     int base_face_id)
 {
+  struct frame *f = WINDOW_XFRAME (w);
   Lisp_Object attrs[LFACE_VECTOR_SIZE];
   struct face *base_face;

@@ -6190,7 +6331,7 @@ merge_faces (struct frame *f, Lisp_Object face_name,
int face_id,
       face_name = lface_id_to_name[face_id];
       /* When called during make-frame, lookup_derived_face may fail
 	 if the faces are uninitialized.  Don't signal an error.  */
-      face_id = lookup_derived_face (f, face_name, base_face_id, 0);
+      face_id = lookup_derived_face (w, f, face_name, base_face_id, 0);
       return (face_id >= 0 ? face_id : base_face_id);
     }

@@ -6199,7 +6340,7 @@ merge_faces (struct frame *f, Lisp_Object face_name,
int face_id,

   if (!NILP (face_name))
     {
-      if (!merge_named_face (f, face_name, attrs, 0))
+      if (!merge_named_face (w, f, face_name, attrs, 0))
 	return base_face_id;
     }
   else
@@ -6210,7 +6351,7 @@ merge_faces (struct frame *f, Lisp_Object face_name,
int face_id,
       face = FACE_FROM_ID_OR_NULL (f, face_id);
       if (!face)
 	return base_face_id;
-      merge_face_vectors (f, face->lface, attrs, 0);
+      merge_face_vectors (w, f, face->lface, attrs, 0);
     }

   /* Look up a realized face with the given face attributes,
@@ -6421,6 +6562,11 @@ syms_of_xfaces (void)
   DEFSYM (Qunspecified, "unspecified");
   DEFSYM (QCignore_defface, ":ignore-defface");

+  /* Used for limiting character attributes to windows with specific
+     characteristics.  */
+  DEFSYM (Qwindow_kw, ":window");
+  DEFSYM (Qfiltered_kw, ":filtered");
+
   /* The symbol `face-alias'.  A symbol having that property is an
      alias for another face.  Value of the property is the name of
      the aliased face.  */
@@ -6496,6 +6642,10 @@ syms_of_xfaces (void)
   defsubr (&Sdump_colors);
 #endif

+  DEFVAR_BOOL ("face-filters-always-match", face_filters_always_match,
+               doc: /* Non-nil means that face filters are always deemed to
+match. Use only when evaluating face attributes.  */);
+
   DEFVAR_LISP ("face-new-frame-defaults", Vface_new_frame_defaults,
     doc: /* List of global face definitions (for internal use only.)  */);
   Vface_new_frame_defaults = Qnil;

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

> This patch lets us actually vary faces on a window-by-window basis.

Sounds good.

> +   4. Conses of the form
> +   (:filter (:window PARAMETER VALUE) FACE-SPECIFICATION),
> +   which applies FACE-SPECIFICATION only if the
> +   given face attributes are being evaluated in the context of a
> +   window with a parameter named PARAMETER being EQ VALUE.

Is there any chance we could allow running Elisp code there, i.e. allow

    (:filter FUNCTION FACE-SPECIFICATION)

or even just

    (:filter FUNCTION)

where FUNCTION returns the FACE-SPECIFICATION (or nil, say).

>    DEFSYM (QCignore_defface, ":ignore-defface");
>
> +  /* Used for limiting character attributes to windows with specific
> +     characteristics.  */
> +  DEFSYM (Qwindow_kw, ":window");
> +  DEFSYM (Qfiltered_kw, ":filtered");

Our convention is to use QC<foo> rather than Q<foo>_kw for ":foo", as
can be seen above for :ignore-defface.


        Stefan

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

On 06/07/2018 07:57 PM, Stefan Monnier wrote:
>> This patch lets us actually vary faces on a window-by-window basis.
> 
> Sounds good.
> 
>> +   4. Conses of the form
>> +   (:filter (:window PARAMETER VALUE) FACE-SPECIFICATION),
>> +   which applies FACE-SPECIFICATION only if the
>> +   given face attributes are being evaluated in the context of a
>> +   window with a parameter named PARAMETER being EQ VALUE.
> 
> Is there any chance we could allow running Elisp code there, i.e. allow
I thought about that, but I worry that allowing arbitrary code to 
execute there would make any kind of caching invalidation or something 
much harder later. ISTM that lisp should always be able to compute 
whatever it wants ahead of time and smuggle the result into redisplay 
via the window parameter.

>      (:filter FUNCTION FACE-SPECIFICATION)
> 
> or even just
> 
>      (:filter FUNCTION)
> 
> where FUNCTION returns the FACE-SPECIFICATION (or nil, say).

If we went this route, I'd probably want to call it :eval.

>>     DEFSYM (QCignore_defface, ":ignore-defface");
>>
>> +  /* Used for limiting character attributes to windows with specific
>> +     characteristics.  */
>> +  DEFSYM (Qwindow_kw, ":window");
>> +  DEFSYM (Qfiltered_kw, ":filtered");
> 
> Our convention is to use QC<foo> rather than Q<foo>_kw for ":foo", as
> can be seen above for :ignore-defface.

Gah, yes, of course.

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

>> Is there any chance we could allow running Elisp code there, i.e. allow
> I thought about that, but I worry that allowing arbitrary code to execute
> there would make any kind of caching invalidation or something much harder
> later.

Indeed.

>>      (:filter FUNCTION FACE-SPECIFICATION)
>> or even just
>>      (:filter FUNCTION)
>> where FUNCTION returns the FACE-SPECIFICATION (or nil, say).
> If we went this route, I'd probably want to call it :eval.

[ Based on the previous point, this might be moot, but it's a problem
  we have in many places in Elisp, so I find it's important to say it:  ]

I don't object to the name :eval BUT the argument should be a *function*
and not an *expression* (i.e. the implementation should involve
`funcall` rather than `eval`).

It's no accident that "eval" and "evil" sound so much alike.


        Stefan

Re: [RFC PATCH] Per-window face support

[Eli Zaretskii]

> Date: Thu, 7 Jun 2018 16:42:30 -0700
> From: dancol@dancol.org
> 
> This patch lets us actually vary faces on a window-by-window basis. It
> works by allowing face-remapping-alist entries to apply only to windows
> with specific parameters set. A package that wants to apply a face
> remapping to all windows with certain characteristics should use
> face-remapping to add a remapping in all buffers, then set window
> parameters to make that remapping take effect some of the time.

Thanks.  I think you should put this on a branch, so that people could
play with the feature and report any issues they find, before we
decide to merge (which I think will be very soon, given the relative
simplicity of the implementation).

A few minor comments below.

> -  /* The purpose of the following code is to signal an error if FACE
> -     is not a face.  This is for the caller's convenience only; the
> -     redisplay code should be able to fail gracefully.  Skip the check
> -     if FRINGE_FACE_ID is unrealized (as in batch mode and during
> -     daemon startup).  */
> -  if (!NILP (face))
> -    {
> -      struct frame *f = SELECTED_FRAME ();
> -
> -      if (FACE_FROM_ID_OR_NULL (f, FRINGE_FACE_ID)
> -	  && lookup_derived_face (f, face, FRINGE_FACE_ID, 1) < 0)
> -	error ("No such face");
> -    }
> -
> +  /* We used to check, as a convenience to callers, for basic face
> +     validity here, but since validity can depend on the specific
> +     _window_ in which this buffer is being displayed, defer the check
> +     to redisplay, which can cope with bad face specifications.  */

How about leaving this check in the code, but only if the face spec
doesn't include a filter?  Is that feasible?

>    /* The default face, possibly remapped. */
> -  default_face = FACE_FROM_ID_OR_NULL (f,
> -				       lookup_basic_face (f, DEFAULT_FACE_ID));
> +  default_face = FACE_FROM_ID_OR_NULL (
> +    f,
> +    lookup_basic_face (it->w, f, DEFAULT_FACE_ID));

This mostly changes whitespace, and the result is not according to our
style, I think.  Maybe move FACE_FROM_ID_OR_NULL to a new line?

> +/* Determine whether the face filter FILTER evaluated in window W
> +   matches.  W can be NULL if the window context is unknown.
> +
> +   A face filter is either nil, which always matches, or a list
> +   (:window PARAMETER VALUE), which matches if the current window has
> +   a PARAMETER EQ to VALUE.
> +
> +   If the filter is invalid, set *OK to false and, if ERR_MSGS is
> +   true, log an error message.  */
> +static bool
> +evaluate_face_filter (Lisp_Object filter, struct window *w,
> +                      bool *ok, bool err_msgs)

I think the commentary should explicitly document the return value,
even though this is a predicate function with a boolean return type.

More importantly, *OK is not touched if the filter is valid.  If this
is intentional, it should be documented, because it requires the
caller to assign a value before calling this function.

> +static bool
> +evaluate_face_filter (Lisp_Object filter, struct window *w,
> +                      bool *ok, bool err_msgs)
> +{
> +  Lisp_Object orig_filter = filter;
> +
> +  {
> +    if (NILP (filter))
> +      return true;

The inner braces seem to be redundant here?

> +    if (w) {
> +      Lisp_Object found = assq_no_quit (parameter, w->window_parameters);
> +      if (!NILP (found) && EQ (XCDR (found), value))
> +        match = true;
> +    }

The style of braces here is not according to our conventions.

Also, I wonder whether EQ should actually be equal_no_quit, because
the latter would allow parameters with values that are not just
integers or symbols.

> +/* Determine whether FACE_REF is a "filter" face specification (case
> +   #4 in merge_face_ref). If it is, evaluate the filter, and if the
> +   filter matches, return the filtered expression. Otherwise, return
> +   the original expression.

The function can also return nil, but that is not documented.

> +   On error, set *OK to false, having logged an error message if

Same issue as above here with the *OK thing.

> +   ERR_MSGS is true, with return value unspecified.
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
If this last part is about returning nil, I'm not sure I see the
reason for being opaque here.  This is an internal function used only
in this one source file; why do we need to hide from potential
programmers its return values in various circumstances?

> +   W is either NULL or a window used to evaluate filters. If W is
> +   null, no window-based face specification filter matches.

Please use 2 spaces between sentences in comments and strings, per our
conventions (here and elsewhere in the patch).

> +static Lisp_Object
> +filter_face_ref (Lisp_Object face_ref,
> +                 struct window *w,
> +                 bool *ok,
> +                 bool err_msgs)
> +{
> +  Lisp_Object orig_face_ref = face_ref;
> +  if (!CONSP (face_ref))
> +    return face_ref;
> +
> +  {

The inner braces are again redundant, I think.

> +   5. nil, which means to merge nothing.

Is this special case needed?  I'm not sure such no-op arguments are a
good idea, as they require to look at the called functions to
understand what's going on on higher levels.  IME it is better to
expect the callers to test for nil explicitly before calling.

>        if (EQ (first, Qforeground_color)
> -	  || EQ (first, Qbackground_color))
> +               || EQ (first, Qbackground_color))

Unsolicited whitespace change?

> +   face isn't realized and cannot be realized.  If window W is given,
> +   consider face remappings specified for W or for W's buffer. If W is
> +   NULL, consider only frame-level face configuration.  */

Two spaces between sentences, please.

> +  DEFVAR_BOOL ("face-filters-always-match", face_filters_always_match,
> +               doc: /* Non-nil means that face filters are always deemed to
> +match. Use only when evaluating face attributes.  */);

The first line of the doc string should be 79 characters at most, so I
suggest to move the second sentence to a new line.

Also, by "use only..." do you mean this should be let-bound or
something?  IOW, I think the doc string should be somewhat more
explicit regarding the restrictions of this variable's use.

Finally, this will need documentation updates in lispref and in NEWS.

Thanks again for working on this.

Re: [RFC PATCH] Per-window face support

[Eli Zaretskii]

> Date: Fri, 08 Jun 2018 12:15:44 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org

Daniel, you never answered my question below:

> > +    if (w) {
> > +      Lisp_Object found = assq_no_quit (parameter, w->window_parameters);
> > +      if (!NILP (found) && EQ (XCDR (found), value))
> > +        match = true;
> > +    }
> 
> [...]
> 
> Also, I wonder whether EQ should actually be equal_no_quit, because
> the latter would allow parameters with values that are not just
> integers or symbols.

I still wonder why we only allow EQ there, it sounds unnecessarily
restrictive.

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

On 06/16/2018 03:29 AM, Eli Zaretskii wrote:
>> Date: Fri, 08 Jun 2018 12:15:44 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: emacs-devel@gnu.org
> 
> Daniel, you never answered my question below:
> 
>>> +    if (w) {
>>> +      Lisp_Object found = assq_no_quit (parameter, w->window_parameters);
>>> +      if (!NILP (found) && EQ (XCDR (found), value))
>>> +        match = true;
>>> +    }
>>
>> [...]
>>
>> Also, I wonder whether EQ should actually be equal_no_quit, because
>> the latter would allow parameters with values that are not just
>> integers or symbols.
> 
> I still wonder why we only allow EQ there, it sounds unnecessarily
> restrictive.

Because there's no need for greater power. All you need is simple 
classification.

Re: [RFC PATCH] Per-window face support

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Daniel Colascione <dancol@dancol.org>
> Date: Sat, 16 Jun 2018 07:24:13 -0700
> 
> >>> +    if (w) {
> >>> +      Lisp_Object found = assq_no_quit (parameter, w->window_parameters);
> >>> +      if (!NILP (found) && EQ (XCDR (found), value))
> >>> +        match = true;
> >>> +    }
> >>
> >> [...]
> >>
> >> Also, I wonder whether EQ should actually be equal_no_quit, because
> >> the latter would allow parameters with values that are not just
> >> integers or symbols.
> > 
> > I still wonder why we only allow EQ there, it sounds unnecessarily
> > restrictive.
> 
> Because there's no need for greater power. All you need is simple 
> classification.

That's not the Emacs way, IMO.  Being able to use only integers or
symbols as filtering parameters sounds too restrictive to me, and I
see no reason for such restrictions.  Also, the code and the
documentation were clearly written with an eye towards future
extensions, which seems to contradict "no need for greater power".

But if I'm the only one who's bothered by these restrictions, then so
be it.

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

>> Cc: emacs-devel@gnu.org
>> From: Daniel Colascione <dancol@dancol.org>
>> Date: Sat, 16 Jun 2018 07:24:13 -0700
>>
>> >>> +    if (w) {
>> >>> +      Lisp_Object found = assq_no_quit (parameter,
>> w->window_parameters);
>> >>> +      if (!NILP (found) && EQ (XCDR (found), value))
>> >>> +        match = true;
>> >>> +    }
>> >>
>> >> [...]
>> >>
>> >> Also, I wonder whether EQ should actually be equal_no_quit, because
>> >> the latter would allow parameters with values that are not just
>> >> integers or symbols.
>> >
>> > I still wonder why we only allow EQ there, it sounds unnecessarily
>> > restrictive.
>>
>> Because there's no need for greater power. All you need is simple
>> classification.
>
> That's not the Emacs way, IMO.  Being able to use only integers or
> symbols as filtering parameters sounds too restrictive to me, and I
> see no reason for such restrictions.

There's no loss of generality in allowing only eq at face filtering time:
you can set an integer or symbol to reflect the result of any other
computation, as I do at
https://github.com/dcolascione/emacs-window-highlight/blob/master/window-highlight.el.

If you want to evaluate a condition more complex than eq can handle, you
arrange to run some code when your condition changes, then store the
result of your comparison in a property that the face filter can inspect
with eq. Anything you can do by allowing equal in filters, you can do
using this technique.

 Also, the code and the
> documentation were clearly written with an eye towards future
> extensions, which seems to contradict "no need for greater power".
>
> But if I'm the only one who's bothered by these restrictions, then so
> be it.

Contractually allowing something more powerful than eq here can really
screw us over later: what happens when we equal-compare large data
structures? What happens when we eventually get custom type-by-type
equality predicates? What happens if we want to cache the result of
applying face filters? The way it is now, with an eq-test, the result of a
face filter operation can change _only_ after a call to
set-window-parameter or an update of a face spec somewhere. I don't want
to give up this invariant for no reason.

Limiting the test to eq greatly simplifies the contract while not reducing
power. _That's_ the Emacs way.

Re: [RFC PATCH] Per-window face support

[Eli Zaretskii]

> Date: Sat, 16 Jun 2018 08:30:52 -0700
> From: "Daniel Colascione" <dancol@dancol.org>
> Cc: "Daniel Colascione" <dancol@dancol.org>,
>  emacs-devel@gnu.org
> 
> > That's not the Emacs way, IMO.  Being able to use only integers or
> > symbols as filtering parameters sounds too restrictive to me, and I
> > see no reason for such restrictions.
> 
> There's no loss of generality in allowing only eq at face filtering time:
> you can set an integer or symbol to reflect the result of any other
> computation

That's exactly the point: one needs to modify window parameters,
instead of just using what's already there.  So this will restrict
applications that want to modify a window's faces based on existing
window parameters whose values are neither symbols nor integers.  Such
applications will have to add their own parameters to windows.

> Contractually allowing something more powerful than eq here can really
> screw us over later: what happens when we equal-compare large data
> structures?

Same thing that happens when an :eval form in the mode-line-format
causes Emacs to solve partial differential equations in 20 dimensions:
they get what they deserve.  Emacs never prevented legitimate uses of
power features just because those features give us enough rope to hang
ourselves, should we be so stupid.

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

>>> > I still wonder why we only allow EQ there, it sounds unnecessarily
>>> > restrictive.

Agree.  A recent discussion indeed pointed out that EQ is fundamentally
flawed and there can be good reasons to try and give up on EQ and use
`equal` instead.

> applying face filters?  The way it is now, with an eq-test, the result
> of a face filter operation can change _only_ after a call to
> set-window-parameter or an update of a face spec somewhere.

Sounds like a good argument for EQ here.

So, from where I stand, you're both right.


        Stefan


PS: BTW, Dan, thinking about how to allow Elisp code in there, maybe one
way would be to make it so the Elisp function returns not just the
remapping but also some kind of "freshness predicate"" which can be used
to determine when the function needs to be re-evaluated.

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

>>>> > I still wonder why we only allow EQ there, it sounds unnecessarily
>>>> > restrictive.
>
> Agree.  A recent discussion indeed pointed out that EQ is fundamentally
> flawed and there can be good reasons to try and give up on EQ and use
> `equal` instead.

Huh? EQ is the simplest, least flawed primitive one can imagine. It's
simple object identity comparison.

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

>>>>> > I still wonder why we only allow EQ there, it sounds unnecessarily
>>>>> > restrictive.
>> Agree.  A recent discussion indeed pointed out that EQ is fundamentally
>> flawed and there can be good reasons to try and give up on EQ and use
>> `equal` instead.
> Huh? EQ is the simplest, least flawed primitive one can imagine. It's
> simple object identity comparison.

In the context of bignums and floats it's pretty thorougly flawed.
[ I said we should use `equal` instead but that was a mistake: I meant `eql`.  ]


        Stefan

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

>>>>>> > I still wonder why we only allow EQ there, it sounds unnecessarily
>>>>>> > restrictive.
>>> Agree.  A recent discussion indeed pointed out that EQ is fundamentally
>>> flawed and there can be good reasons to try and give up on EQ and use
>>> `equal` instead.
>> Huh? EQ is the simplest, least flawed primitive one can imagine. It's
>> simple object identity comparison.
>
> In the context of bignums and floats it's pretty thorougly flawed.
> [ I said we should use `equal` instead but that was a mistake: I meant
> `eql`.  ]

I think that ship sailed a long time ago. Almost everyone uses eq for
integers, so we should, I think, intern bignums so that things keep
working.

Floats typically aren't equality-compared anyway.

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

> I think that ship sailed a long time ago. Almost everyone uses eq for
> integers, so we should, I think, intern bignums so that things keep
> working.

But using `eql` is a lot cheaper than hash-consing all bignums
and floats.

I think we should simply start to use `eql` more and phase out `eq`
little by little.


        Stefan

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

>> I think that ship sailed a long time ago. Almost everyone uses eq for
>> integers, so we should, I think, intern bignums so that things keep
>> working.
>
> But using `eql` is a lot cheaper than hash-consing all bignums
> and floats.

We should consider floats and bignums separately. We get along fine today
with floats the way they are. Bignums are different because we'd be
changing operations that would overflow today into operations that return
bignums, so users would get bignums in places they don't expect. That's
why eq has to keep working.

Anyway, I still think hash-consing bignums is fine, because it's
pay-for-what-you-use. Bignums are expensive anyway, so the overhead of
interning them shouldn't matter all that much. I bet it's cheaper to
intern the occasional bignum than to add branches to every eq in the
codebase.

> I think we should simply start to use `eql` more and phase out `eq`
> little by little.

Sure. Part of the problem is that primitives like assq are hardcoded for
eq-comparison, and other primitives, like assoc, are hardcoded for
equal-comparison. We could add more primitives, but I don't think it'd be
worth the trouble.

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

> Sure. Part of the problem is that primitives like assq are hardcoded for
> eq-comparison,

We can change `assq` to use EQL: any code affected by this change is
fundamentally broken anyway (for the same reason that (defalias 'eq
#'eql) should be safe).

> We could add more primitives,

No need, on the contrary: by phasing out `eq` we reduce the traditional
3 forms of equality to just 2.


        Stefan

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

>> Sure. Part of the problem is that primitives like assq are hardcoded for
>> eq-comparison,
>
> We can change `assq` to use EQL: any code affected by this change is
> fundamentally broken anyway (for the same reason that (defalias 'eq
> #'eql) should be safe).
>
>> We could add more primitives,
>
> No need, on the contrary: by phasing out `eq` we reduce the traditional
> 3 forms of equality to just 2.

I strongly object. Getting rid of `eq' is just lying to programmers about
object identity.

Re: [RFC PATCH] Per-window face support

[Paul Eggert]

Daniel Colascione wrote:
> Getting rid of `eq' is just lying to programmers about
> object identity.

I don't see how it would be lying to equate eq and eql for numbers. An object's 
identity is not the same thing as a machine-level address, and whether two 
instances of the same number are eq is an implementation detail that Lisp 
programmers should not rely upon.

This doesn't mean that we should equate eq and eql. Perhaps there are good 
efficiency reasons to continue to distinguish them. But these would be merely 
pragmatic, not philosophical.

Aliasing EQ to EQL (was: [RFC PATCH] Per-window face support)

[Stefan Monnier]

>> Getting rid of `eq' is just lying to programmers about object identity.
>
> I don't see how it would be lying to equate eq and eql for numbers.  An
> object's identity is not the same thing as a machine-level address, and
> whether two instances of the same number are eq is an implementation detail
> that Lisp programmers should not rely upon.
>
> This doesn't mean that we should equate eq and eql. Perhaps there are good
> efficiency reasons to continue to distinguish them. But these would be
> merely pragmatic, not philosophical.

BTW, if someone is tempted to measure the impact, here's the naive patch
I've been using recently.

It doesn't try to be clever: other than NILP, all uses of EQ are changed
to use EQL.  Clearly, we could improve on that, but I think such
improvements should be "profile-guided".

In terms of code size it added 100KB (out of a 4MB stripped binary, so
about 0.25%) which is not insignificant, but is a cost I'm willing to
live with.


        Stefan


diff --git a/src/fns.c b/src/fns.c
index de1dad3736..11709ae266 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2108,10 +2108,7 @@ DEFUN ("eql", Feql, Seql, 2, 2, 0,
 Floating-point numbers of equal value are `eql', but they may not be `eq'.  */)
   (Lisp_Object obj1, Lisp_Object obj2)
 {
-  if (FLOATP (obj1))
-    return equal_no_quit (obj1, obj2) ? Qt : Qnil;
-  else
-    return EQ (obj1, obj2) ? Qt : Qnil;
+  return EQL (obj1, obj2) ? Qt : Qnil;
 }
 
 DEFUN ("equal", Fequal, Sequal, 2, 2, 0,
@@ -2192,7 +2189,7 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 	}
     }
 
-  if (EQ (o1, o2))
+  if (lisp_h_EQ (o1, o2))
     return true;
   if (XTYPE (o1) != XTYPE (o2))
     return false;
@@ -3684,7 +3681,7 @@ cmpfn_user_defined (struct hash_table_test *ht,
    in a Lisp integer.  */
 
 static EMACS_UINT
-hashfn_eq (struct hash_table_test *ht, Lisp_Object key)
+hashfn__eq (struct hash_table_test *ht, Lisp_Object key)
 {
   return XHASH (key) ^ XTYPE (key);
 }
@@ -3706,7 +3703,13 @@ hashfn_equal (struct hash_table_test *ht, Lisp_Object key)
 static EMACS_UINT
 hashfn_eql (struct hash_table_test *ht, Lisp_Object key)
 {
-  return FLOATP (key) ? hashfn_equal (ht, key) : hashfn_eq (ht, key);
+  return FLOATP (key) ? hashfn_equal (ht, key) : hashfn__eq (ht, key);
+}
+
+static EMACS_UINT
+hashfn_eq (struct hash_table_test *ht, Lisp_Object key)
+{
+  return hashfn_eql (ht, key);
 }
 
 /* Value is a hash code for KEY for use in hash table H which uses as
diff --git a/src/lisp.h b/src/lisp.h
index 56ad8b814b..e3b2cd9cf0 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -322,7 +322,7 @@ error !;
 #define lisp_h_INTEGERP(x) ((XTYPE (x) & (Lisp_Int0 | ~Lisp_Int1)) == Lisp_Int0)
 #define lisp_h_MARKERP(x) (MISCP (x) && XMISCTYPE (x) == Lisp_Misc_Marker)
 #define lisp_h_MISCP(x) (XTYPE (x) == Lisp_Misc)
-#define lisp_h_NILP(x) EQ (x, Qnil)
+#define lisp_h_NILP(x) lisp_h_EQ (x, Qnil)
 #define lisp_h_SET_SYMBOL_VAL(sym, v) \
    (eassert ((sym)->u.s.redirect == SYMBOL_PLAINVAL), \
     (sym)->u.s.val.value = (v))
@@ -374,7 +374,7 @@ error !;
 # define CHECK_SYMBOL(x) lisp_h_CHECK_SYMBOL (x)
 # define CHECK_TYPE(ok, predicate, x) lisp_h_CHECK_TYPE (ok, predicate, x)
 # define CONSP(x) lisp_h_CONSP (x)
-# define EQ(x, y) lisp_h_EQ (x, y)
+# define EQ(x, y) EQL (x, y)
 # define FLOATP(x) lisp_h_FLOATP (x)
 # define INTEGERP(x) lisp_h_INTEGERP (x)
 # define MARKERP(x) lisp_h_MARKERP (x)
@@ -1040,10 +1040,21 @@ make_natnum (EMACS_INT n)
 
 /* Return true if X and Y are the same object.  */
 
+extern bool equal_no_quit (Lisp_Object o1, Lisp_Object o2);
+
+INLINE bool
+EQL (Lisp_Object x, Lisp_Object y)
+{
+  if (lisp_h_FLOATP (x))
+    return equal_no_quit (x, y);
+  else
+    return lisp_h_EQ (x, y);
+}
+
 INLINE bool
 (EQ) (Lisp_Object x, Lisp_Object y)
 {
-  return lisp_h_EQ (x, y);
+  return EQL (x, y);
 }
 
 /* True if the possibly-unsigned integer I doesn't fit in a Lisp fixnum.  */
@@ -3432,7 +3443,6 @@ extern Lisp_Object merge (Lisp_Object, Lisp_Object, Lisp_Object);
 extern Lisp_Object do_yes_or_no_p (Lisp_Object);
 extern Lisp_Object concat2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object concat3 (Lisp_Object, Lisp_Object, Lisp_Object);
-extern bool equal_no_quit (Lisp_Object, Lisp_Object);
 extern Lisp_Object nconc2 (Lisp_Object, Lisp_Object);
 extern Lisp_Object assq_no_quit (Lisp_Object, Lisp_Object);
 extern Lisp_Object assoc_no_quit (Lisp_Object, Lisp_Object);

Re: Aliasing EQ to EQL (was: [RFC PATCH] Per-window face support)

[Daniel Colascione]

>>> Getting rid of `eq' is just lying to programmers about object identity.
>>
>> I don't see how it would be lying to equate eq and eql for numbers.  An
>> object's identity is not the same thing as a machine-level address, and
>> whether two instances of the same number are eq is an implementation
>> detail
>> that Lisp programmers should not rely upon.
>>
>> This doesn't mean that we should equate eq and eql. Perhaps there are
>> good
>> efficiency reasons to continue to distinguish them. But these would be
>> merely pragmatic, not philosophical.
>
> BTW, if someone is tempted to measure the impact, here's the naive patch
> I've been using recently.
>
> It doesn't try to be clever: other than NILP, all uses of EQ are changed
> to use EQL.  Clearly, we could improve on that, but I think such
> improvements should be "profile-guided".
>
> In terms of code size it added 100KB (out of a 4MB stripped binary, so
> about 0.25%) which is not insignificant, but is a cost I'm willing to
> live with.

For what? This aliasing has _zero_ real world benefit. I maintain that
this is a change we should absolutely not make.

Re: Aliasing EQ to EQL

[Stefan Monnier]

> For what?  This aliasing has _zero_ real world benefit.

The benefit (cleaner, simpler language) is marginal, indeed.

> I maintain that this is a change we should absolutely not make.

I have no intention to push this change.
But I think eventually we will make a change along those lines.

For that reason, when I see an argument between `eq` vs `equal`, I tend
to translate it into `eql` vs `equal`.


        Stefan

Re: Aliasing EQ to EQL

[Daniel Colascione]

>> For what?  This aliasing has _zero_ real world benefit.
>
> The benefit (cleaner, simpler language) is marginal, indeed.
>
>> I maintain that this is a change we should absolutely not make.
>
> I have no intention to push this change.
> But I think eventually we will make a change along those lines.

Maybe once we have a JIT and can reasonably infer in the right context
that in order to implement EQL-comparison, we need just check object
identity.

Re: Aliasing EQ to EQL

[Stefan Monnier]

> Maybe once we have a JIT and can reasonably infer in the right context
> that in order to implement EQL-comparison, we need just check object
> identity.

[ Hmm... technically, in my dictionary, `eql` checks object identity and
  `eq` checks pointer equality.  ]

Not sure which will be the driving force, but I'd be surprised if 20
years from now we still have `eq` different from `eql`.


        Stefan

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

> I think that ship sailed a long time ago. Almost everyone uses eq for
> integers,

That's OK.  We can (and probably should)

    (defalias 'eq #'eql)

without any significant performance impact.  Doing such a

    #define EQ EQL

in the C code might be too costly, which I why I suggest to do this
bit-by-bit.


        Stefan

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

>> I think that ship sailed a long time ago. Almost everyone uses eq for
>> integers,
>
> That's OK.  We can (and probably should)
>
>     (defalias 'eq #'eql)
>
> without any significant performance impact.  Doing such a
>
>     #define EQ EQL
>
> in the C code might be too costly, which I why I suggest to do this
> bit-by-bit.

That's even worse, because now eq will mean different things depending on
whether a routine is implemented in C or Lisp, and Lisp would have no way
to do _real_ object identity comparisons. This isn't a problem we need to
solve so clumsily.

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

> and Lisp would have no way to do _real_ object identity comparisons.

`eql` works just fine for object identity comparison.


        Stefan

Re: [RFC PATCH] Per-window face support

[Daniel Colascione]

>> and Lisp would have no way to do _real_ object identity comparisons.
>
> `eql` works just fine for object identity comparison.

If it did, we wouldn't need both eq and eql.

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

>>> and Lisp would have no way to do _real_ object identity comparisons.
>> `eql` works just fine for object identity comparison.
> If it did, we wouldn't need both eq and eql.

I agree with you trivially because I contend that we don't need both eq and eql.
The definition of `eq` in Common-Lisp (and Scheme likewise for `eq?`)
allows it to be defined identically to `eql`.

Doing so leads to a simpler API for the Elisp programmer.


        Stefan

Re: [RFC PATCH] Per-window face support

[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. ]]]

  > The definition of `eq` in Common-Lisp (and Scheme likewise for `eq?`)
  > allows it to be defined identically to `eql`.

I fear this would lead to a big slowdown in assq, memq, and many other
inner loops that now check EQ.  Replacing that with EQL, even if it is
open-coded, would require testing the type first.

One way to avoid that slowdown would be to replace each inner loop
with two inner loops, one for the case where EQL really means EQ
and one for the case where it doesn't.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: [RFC PATCH] Per-window face support

[Stefan Monnier]

> I fear this would lead to a big slowdown in assq, memq, and many other
> inner loops that now check EQ.  Replacing that with EQL, even if it is
> open-coded, would require testing the type first.

I doubt it'd be "big", but yes, it probably slows things down a bit.
I'm using such a patch and haven't noticed any slowdown, but I admit
I haven't bothered to try and measure it.


        Stefan