Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii]

> Cc: address@hidden
> From: Paul Eggert <address@hidden>
> Date: Sat, 25 Jun 2016 23:34:52 +0200
> 
> >   . Define FACE_FROM_ID as it was before:
> >
> >     #define FACE_FROM_ID(F, ID)                             \
> >     (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)       \
> >      ? FRAME_FACE_CACHE (F)->faces_by_id[ID]                \
> >      : NULL)
> >
> >   . In every place that calls FACE_FROM_ID and doesn't already include
> >     code to deal with a NULL value, do this:
> >
> >       face = FACE_FROM_ID (f, face_id);
> >       if (!face)
> >         emacs_abort ();
> >
> >   . Remove FACE_OPT_FROM_ID
> >
> 
> If we go this route, I suggest having a function NONNULL_FACE_FROM_ID 
> that packages up those three lines of code (which will occur in many 
> places). That is, instead of this:
> 
>    face = FACE_FROM_ID (f, face_id);
>    if (!face)
>      emacs_abort ();
> 
> we should write this:
> 
>    face = NONNULL_FACE_FROM_ID (f, face_id);
> 
> to mean the same thing. (Or perhaps you can think of a better name than 
> NONNULL_FACE_FROM_ID.)
> 
> This sort of thing should also suffice to remove the warning. A downside 
> is that it inserts unnecessary runtime checks in production code, as the 
> "if (!face) emacs_abort ();" won't detect errors in production usefully 
> earlier than the existing code does when it dereferences the pointer. In 
> practice the unnecessary checks probably won't cost that much, so it's 
> OK if nobody else cares about the performance degradation in production 
> code. (I tried hard to avoid slowing down Emacs merely to pacify GCC, 
> and if we go this route we'd be departing slightly from that tradition.)

OK, thanks.  I took all these arguments under consideration, and made
the corresponding changes in master commit 55d38fc.  Most of it is
just renaming FACE_OPT_FROM_ID to FACE_FROM_ID_OR_NULL (the latter is
IMO more telling about its purpose), but some of the changes change
the macro we call, either because the calling code can cope with a
NULL face pointer, or because it cannot, but the dereference was too
far for GCC 6 to notice it.

I decided not to abort when ENABLE_CHECKING is not defined, because
you convinced me that is unnecessary.