Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Paul Eggert]

On 06/24/2016 10:49 AM, Eli Zaretskii wrote:
> Yes, and on purpose. If the face pointer cannot be determined from its 
> ID, we have no other alternative but crash (or abort) anyway.

Of course, and FACE_FROM_ID is intended to be used in situations like 
this. However, in some cases the caller can handle the situation where a 
face pointer cannot be determined from its ID, and FACE_OPT_FROM_ID is 
intended to be used in those cases. It is helpful to the reader (at 
least, to this reader) to easily see which case is which.

> When the face ID is not a valid value, as tested against 
> FRAME_FACE_CACHE (F)->used, ... FACE_FROM_ID(F, ID), ... when compiled 
> without --enable-checking, will index the FRAME_FACE_CACHE 
> (F)->face_by_id[] array with an invalid index, and either segfault or 
> produce some random garbage that will cause trouble elsewhere. So we 
> are not solving a real problem here

?! Yes we are. The eassert is helpful, since it reliably crashes Emacs 
when ID is out-of-range, something that is useful when debugging. On all 
platforms I know of there is no need to add an eassert that the 
resulting pointer is non-null, since the caller is about to dereference 
it anyway and that will reliably cause a crash. However, it is helpful 
to eassert that ID is in range, since most modern platforms lack 
reliable subscript checking.

With FACE_FROM_ID, the ID should never be out-of-range if Emacs is 
written correctly, so it's OK to omit the runtime check from production 
code, for performance. This is not true for FACE_OPT_FROM_ID; it is 
supposed to return NULL when ID is out of range, and callers are 
supposed to avoid dereferencing the resulting pointer if it is NULL.

> just shutting up the (stupid, IMO) warning from a compiler

It is not a stupid warning. It is a useful warning. The modern trend in 
statically-typed languages is to distinguish "possibly-null pointer to 
X" from "non-null pointer to X". That way, a compile-time check can 
reliably detect the error of dereferencing null pointers, which is a 
real problem in many applications (including Emacs). C does not have 
this notion directly and in general a C compiler cannot detect the error 
statically. That being said, GCC has reasonable heuristics to check for 
the error in many cases, and it's useful to enable this checking to 
catch silly programming errors. (Of course code that you and I write 
would never have these errors; it's always *other* people's code. :-)

> eassert (face); if (!face) { ...}

eassert (X) means that X must be nonzero, so there should never be a 
need for a runtime check !X after a call to eassert (X).