Re: 'struct window' cleanup #2

[Eli Zaretskii <eliz@gnu.org>]

> Date: Wed, 27 Jun 2012 09:06:37 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: dmantipov@yandex.ru, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
>  > Why a "hack"?  Lisp object are good precisely for this reason: that
>  > you can give them values of different types of object.
> 
> window_end_valid is documented as
> 
>      /* t if window_end_pos is truly valid.
>         This is nil if nontrivial redisplay is preempted
>         since in that case the frame image that window_end_pos
>         did not get onto the frame.  */
>      Lisp_Object window_end_valid;
> 
> but the code in xdisp.c checks it usually just as
> 
>       !NILP (w->window_end_valid)
> 
> Consequently, the assignment
> 
>        w->window_end_valid = w->buffer;
> 
> looks fragile (one has to be sure that the NILP checks don't happen
> while it's set to the buffer since otherwise we end up trying again for
> no use) and at least contradicts the initial comment of try_window_id
> 
> /* Try to redisplay window W by reusing its existing display.  W's
>     current matrix must be up to date when this function is called,
>     i.e. window_end_valid must not be nil.
> 
> but apparently the current matrix is not up to date when the value is
> non-nil but some other buffer.  So unless this is better documented
> and/or the corresponding code cleaned up it remains a "hack" IMHO.

What you see happens a lot in Emacs sources: comments don't correspond
to what the code really does, and tell only part of the story.
Usually, someone changes the code, but fails to update the comments
(because these comments are in another file, where the struct is
defined, not where the change is being done).  I tripped on such
issues several times when working on display features (in conjunction
with the various flags and state variables in 'struct it').

That's why I asked Dmitry to update and improve the comments whenever
some variable or struct member is being updated/modified.

>  >>  Can you see whether the part
>  >>
>  >>        /* When splitting windows or for new windows, it happens that
>  >> 	 redisplay is called with a nil window_end_vpos or one being
>  >> 	 larger than the window.  This should really be fixed in
>  >> 	 window.c.  I don't have this on my list, now, so we do
>  >> 	 approximately the same as the old redisplay code.  --gerd.  */
>  >>        && INTEGERP (w->window_end_vpos)
>  >>
>  >> in xdisp.c still makes sense?
>  >
>  > No, there's no code anymore that sets it to nil.
> 
> Fine (apparently that was another "hack" in redisplay).

No, it's another instance of "change-code-but-don't-update-comments"
phenomenon.