Re: face-remapping patch

[Miles Bader]

On Wed, May 28, 2008 at 11:54 AM, Stefan Monnier
<address@hidden> wrote:
> 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.

There's only one such test.   :-)

Anyway, the default value is nil, and while modes and users might
start using it, still, I think it would be nil very commonly.

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

Can't exactly say what's going on here, but this code is pretty old,
so it's possible the trunk code has been changed since the ChangeLog
entry was written.  Indeed I remember being annoyed at the conflicts
that arose when a bunch of that code got re-arranged at some point....
 :-)

>> +  switch (face_id)
>> +    {
>> +    case DEFAULT_FACE_ID:            name = Qdefault;                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?

Yeah it's a bug; callers to that function are only supposed to pass
one of the "basic" face ids.

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

Insofar as I remember, yeah, just an "optimization".  The assumption
is that 99% of the time, there's no remapping (and for most of those
faces, it's probably true), and I think this function is called a lot.

-Miles

-- 
Do not taunt Happy Fun Ball.