Re: FACE_FROM_ID vs FACE_OPT_FROM_ID

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

On 06/23/2016 10:03 PM, Eli Zaretskii wrote:
> What was the GCC 6.1 complaint that led you to this change?
Diagnostics like this one (there were quite a few):

> xdisp.c: In function 'fill_image_glyph_string':
> xdisp.c:24916:20: error: potential null pointer dereference 
> [-Werror=null-dereference]
>    s->font = s->face->font;
>              ~~~~~~~^~~~~~

The problem being that s->face was set from FACE_FROM_ID, which (before 
the change) might return a null pointer.

The intent is that FACE_FROM_ID returns a face (a non-null pointer), 
whereas FACE_OPT_FROM_ID returns a face option (either a face or a null 
pointer). Similarly for IMAGE_FROM_ID. I installed the attached patch to 
try to make this clearer.

[-- Attachment #2: 0001-Clarify-intent-of-FACE_FROM_ID-and-IMAGE_FROM_ID.txt --]
[-- Type: text/plain, Size: 2012 bytes --]

From b3ef03f646c8fb3dc6d55a8ed0134b6963243f86 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 24 Jun 2016 02:19:13 +0200
Subject: [PATCH] Clarify intent of FACE_FROM_ID and IMAGE_FROM_ID
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/dispextern.h (FACE_OPT_FROM_ID): Don’t use FACE_FROM_ID,
since it is intended to be used only when it returns a non-null
pointer, and here the pointer might be null.
(IMAGE_OPT_FROM_ID): Don’t use IMAGE_FROM_ID, for similar reasons.
---
 src/dispextern.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index d0fc3b2..08dcd89 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1812,7 +1812,7 @@ struct face_cache
   bool_bf menu_face_changed_p : 1;
 };
 
-/* Return a pointer to the cached face with ID on frame F.  */
+/* Return a non-null pointer to the cached face with ID on frame F.  */
 
 #define FACE_FROM_ID(F, ID)					\
   (eassert (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)),	\
@@ -1823,7 +1823,7 @@ struct face_cache
 
 #define FACE_OPT_FROM_ID(F, ID)				\
   (UNSIGNED_CMP (ID, <, FRAME_FACE_CACHE (F)->used)	\
-   ? FACE_FROM_ID (F, ID)				\
+   ? FRAME_FACE_CACHE (F)->faces_by_id[ID]		\
    : NULL)
 
 /* True if FACE is suitable for displaying ASCII characters.  */
@@ -3093,7 +3093,7 @@ struct image_cache
 };
 
 
-/* A pointer to the image with id ID on frame F.  */
+/* A non-null pointer to the image with id ID on frame F.  */
 
 #define IMAGE_FROM_ID(F, ID)					\
   (eassert (UNSIGNED_CMP (ID, <, FRAME_IMAGE_CACHE (F)->used)),	\
@@ -3104,7 +3104,7 @@ struct image_cache
 
 #define IMAGE_OPT_FROM_ID(F, ID)				\
   (UNSIGNED_CMP (ID, <, FRAME_IMAGE_CACHE (F)->used)		\
-   ? IMAGE_FROM_ID (F, ID)					\
+   ? FRAME_IMAGE_CACHE (F)->images[ID]				\
    : NULL)
 
 /* Size of bucket vector of image caches.  Should be prime.  */
-- 
2.5.5