Re: face-remapping patch

[Stefan Monnier]

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