Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Eli Zaretskii <eliz@gnu.org>]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 24 Jun 2016 13:17:39 +0200
> 
> On 06/24/2016 12:00 PM, Eli Zaretskii wrote:
> >> my reading of this:
> >>
> >> #ifndef ENABLE_CHECKING
> >> # define eassert(cond) ((void) (false && (cond))) /* Check COND compiles.  */
> >>
> >> is that when ENABLE_CHECKING is not defined, eassert does nothing
> >> useful.
> 
> Yes, of course.  eassert (X) should appear only in places where X must 
> be true, i.e., where Emacs is seriously broken if X is false. That's the 
> standard meaning for assertions.

And that is what happens here: if the face ID is not in the face
cache, something is broken, and we cannot continue using that face ID.

In the ENABLE_CHECKING case, we abort immediately, since that's the
meaning of ENABLE_CHECKING: allow early detection of invalid data.
When ENABLE_CHECKING is not defined, which is what happens by default
in Emacs builds, we attempt to "do the best we can without crashing",
thus my suggestion to use the default face, if that is in the cache.
If even that is not in the cache, we cannot display anything, so I
suggest we abort.

If you find the code I proposed after eassert confusing, then we can
condition it on ENABLE_CHECKING being not defined.

Otherwise, I think my suggestion is better than what we have now,
because the current code, when compiled without ENABLE_CHECKING, will
attempt to use invalid face IDs.

> Another way to put it is that in general the behavior of eassert (X) is 
> undefined if X is false.When (defined ENABLE_CHECKING && 
> !suppress_checking), this undefined behavior happens to be a diagnostic 
> and core dump. Otherwise the undefined behavior is whatever the 
> underlying system does afterwards; when (!defined ENABLE_CHECKING) this 
> yields better performance overall in the usual and expected case where 
> Emacs is working properly.

My suggestion was an attempt to make the undefined behavior safer, as
much as it's possible.  That is in line with the general practice in
Emacs: to avoid crashing as much as possible, leaving the user in
charge, so she could shut down the session in an orderly fashion, thus
minimizing the risk of losing precious work.

> It is not necessary to put eassert (X) every place where an expression X 
> must be true. It's helpful only when we reasonably suspect there might 
> be a bug in Emacs, and where platforms typically will not immediately 
> fail for other reasons when X is false. So, before a pointer dereference 
> *P it's not necessary to do eassert (P != NULL) since typical platforms 
> immediately dump core when dereferencing a null pointer anyway. 

That accurately describes the FACE_FROM_ID macro we had before these
changes, and the way we used it.  We could keep that code, but you say
the GCC warning about that is useful, so I suggested a way of avoiding
the warning that also makes our code safer.  After all, if the warning
is useful, we should indeed make changes that improve our code, not
just shut up the warning.

> Conversely, before a subscript operation A[I] it can be helpful to 
> eassert (0 <= I && I < N), since typical platforms lack reliable 
> subscript checking.

And that's exactly what is missing from current uses of FACE_FROM_ID,
when ENABLE_CHECKING is not defined.

So, turning back to my suggestion, do you still object to it?