Re: Question about display engine

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sun, 29 Sep 2019 12:30:34 +0200
> From: Ergus <spacibba@aol.com>
> Cc: rudalics@gmx.at, emacs-devel@gnu.org
> 
> > . I don't understand why you changed the underlined_p and
> >   underline_type stuff in struct face.  What were the reasons for
> >   that part of the changeset?  Its sole effect seems to be to
> >   generate some redundant changes, or am I missing something?
> >
> Use double variable to describe the same is an error prone
> practice. This change simplifies the checks in all the code related (as
> there will be only one variable to set/check), reduces one name and
> member in the struct and avoids errors related to changing one value and
> not the other.

That's your stylistic preference, and I can understand it.  But when
the only reason for a change is stylistic preferences, I generally
prefer to leave the code intact, so that the original authors' version
remains as long as it does TRT.

> > . In this hunk from append_space_for_newline, why did you change
> >   FACE_FROM_ID_OR_NULL to FACE_FROM_ID?
> >
> >     -         int local_default_face_id =
> >     +      int char_width = 1;
> >     +
> >     +      if (default_face_p
> >     +#ifdef HAVE_WINDOW_SYSTEM
> >     +          || FRAME_WINDOW_P (it->f)
> >     +#endif
> >     +         )
> >     +       {
> >     +         const int local_default_face_id =
> >		 lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID);
> >	       struct face* default_face =
> >     -           FACE_FROM_ID_OR_NULL (it->f, local_default_face_id);
> >     +           FACE_FROM_ID (it->f, local_default_face_id);
> >
> 
> The call is made after a lookup_basic_face and it's for DEFAULT_FACE_ID
> is it needed the check? Normally for other faces if it is NULL then we
> use the DEFAULT_FACE_ID. In this case it should be unneeded right?

It's the other way around: FACE_FROM_ID could trigger an assertion
violation, where FACE_FROM_ID_OR_NULL will silently return a NULL
pointer.  So we should only use FACE_FROM_ID where we are 110% certain
it can never violate the assertion, or where a NULL pointer for a face
will cause worse problems than an assertion.

Thanks.