[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Question about display engine

From: Ergus
Subject: Re: Question about display engine
Date: Mon, 7 Oct 2019 17:40:55 +0200
User-agent: NeoMutt/20180716

Hi Eli:

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

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?

On Sun, Sep 29, 2019 at 01:57:22PM +0300, Eli Zaretskii wrote:
Date: Sun, 29 Sep 2019 12:30:34 +0200
From: Ergus <address@hidden>
Cc: address@hidden, address@hidden

> . 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
>     -         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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]