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 23:34:56 +0200
> 
> On 06/24/2016 03:43 PM, Eli Zaretskii wrote:
> > 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",
> No, assertions should be always true; if an assertion fails Emacs should 
> just abort, and should not try to patch around the bug.

That's not how we've been using eassert in Emacs.  Just look around,
and you will see it.

The current FACE_FROM_ID doesn't do that, either, when compiled with
ENABLE_CHECKING not defined.

We could, of course, make validate_face I proposed unconditionally
call emacs_abort.  Or we could add such code in-line in all places
that don't already have code to deal with a NULL value coming out of
FACE_FROM_ID.  Would you agree to such a change?

> It could be that the font and image caches are special cases that have a 
> lot of bugs, and that we therefore want some sort of belt-and-suspenders 
> treatment for them that goes beyond ordinary assertions. I don't see 
> offhand why that would be, though. Are we getting an unusual number of 
> bugs in that area, and if so what sort of bugs are they?

I don't know how to measure "unusual", so I cannot answer that
question.  I can tell that there were a couple of subtle problems in
the display code that under some rare and complicated use cases caused
FACE_FROM_ID to yield NULL where the subsequent code didn't expect
that.  I believe the known cases are now solved in the development
sources, but I won't be surprised to hear about similar problems that
still exist in even more rare use cases.

As I wrote above, I'm okay with unconditionally aborting if
FACE_FROM_ID ever yields NULL, and the code after that is written
under the assumption that it cannot happen.  However, the current
FACE_FROM_ID doesn't ensure that, so we should make some follow-up
changes to ensure that.

Also, I don't want two macros (and don't see how this could be
achieved by 2 macros anyway).  I read the related code too frequently
to allow confusing macros into it.

So, to summarize, here's the proposal.

 . 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

OK?