Re: face-remapping patch

[Stefan Monnier <monnier@iro.umontreal.ca>]

> I've attached my face-remapping patch to this message (I needed to
> untangle it from some other changes).

> I've used regularly in my personal emacs for many years, and in light
> testing the patch seems to work fine against the current trunk.

It looks pretty good.  Is there any reason why you added redundant tests
like !NILP (Vface_remapping_alist)?  I'd expect at least in my case that
face-remapping-alist would almost never be nil so this extra test would
end up redundant.


        Stefan


> +int
> +lookup_named_face (f, symbol, signal_p)
> +     struct frame *f;
> +     Lisp_Object symbol;
> +     int signal_p;
> +{
> +  return lookup_named_face_1 (f, symbol, 0);
> +}

Why have a `signal_p' arg that's not used?
Also, the ChangeLog entry doesn't make much sense:

	(lookup_named_face_1): Renamed from `lookup_named_face'.  Add
	`signal_p' argument.  Pass new last arg to `get_lface_attributes', and
	return -1 if it fails.
	(lookup_named_face): Redefined in terms of `lookup_named_face_1'.

The signal_p arg was there before, it's not added.  Am I missing something?

> +  switch (face_id)
> +    {
> +    case DEFAULT_FACE_ID:		name = Qdefault;		break;
> +    case MODE_LINE_FACE_ID:		name = Qmode_line;		break;
> +    case MODE_LINE_INACTIVE_FACE_ID:	name = Qmode_line_inactive;	break;
> +    case HEADER_LINE_FACE_ID:		name = Qheader_line;		break;
> +    case TOOL_BAR_FACE_ID:		name = Qtool_bar;		break;
> +    case FRINGE_FACE_ID:		name = Qfringe;			break;
> +    case SCROLL_BAR_FACE_ID:		name = Qscroll_bar;		break;
> +    case BORDER_FACE_ID:		name = Qborder;			break;
> +    case CURSOR_FACE_ID:		name = Qcursor;			break;
> +    case MOUSE_FACE_ID:			name = Qmouse;			break;
> +    case MENU_FACE_ID:			name = Qmenu;			break;
> +
> +    default:
> +      return face_id;		/* Give up.  */

Does this default case correspond to a bug?  if so shouldn't it `abort'?
If not, shouldn't it obey Vface_remapping_alist?

> +  mapping = assq_no_quit (name, Vface_remapping_alist);
> +  if (NILP (mapping))
> +    return face_id;		/* Give up.  */
> +
> +  remapped_face_id = lookup_named_face_1 (f, name, 0);

This looks odd: we look up Vface_remapping_alist and then ignore the
actual result (other than its being null or not).  Is it just an
optimization to avoid calling lookup_named_face_1?  If so, a comment is
in order.