Re: Question about display engine

[Ergus <spacibba@aol.com>]

Hi Eli:

I made some of the fixes you suggested in the last email about this
topic.

The reduction of the underline-p + underline-stile into a single
underline variable in the struct for me still seems like a good change
for readability and simplification in all the code. Any way in the
future I will try to avoid this kind of modifications even when they
seems to be an improvement... but could you let it pass this time?

The other changes that seemed to be stylistic too, were actually in
portions of code I wrote for dfci and It was there because I copied some
of it from the previous if-else section... but I can correct it now
right?... so no problem?

If this is fine may I merge in master?
Best,
Ergus.


On Sun, Sep 29, 2019 at 01:57:22PM +0300, Eli Zaretskii wrote:
>> 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.
>