Re: 'struct window' cleanup #4

[Eli Zaretskii <eliz@gnu.org>]

> Date: Mon, 02 Jul 2012 14:45:02 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> 
> --- src/dispnew.c	2012-06-28 07:50:50 +0000
> +++ src/dispnew.c	2012-07-02 10:11:48 +0000
> @@ -643,9 +643,8 @@
>  
>  	      /* Window end is invalid, if inside of the rows that
>  		 are invalidated below.  */
> -	      if (INTEGERP (w->window_end_vpos)
> -		  && XFASTINT (w->window_end_vpos) >= i)
> -		w->window_end_valid = Qnil;
> +	      if (w->window_end_vpos >= i)
> +		w->window_end_valid = 0;

Shouldn't we test w->window_end_valid for being non-zero, before we
trust the value of w->window_end_vpos?  Zero for a vertical position
is perfectly valid, so if i is zero, you can err here.  The comment
says:

     /* Glyph matrix row of the last glyph in the current matrix of W.
        Should be nonnegative, and valid only if window_end_valid is not zero.  */
     ptrdiff_t window_end_vpos;

So I think we should put our money where our mouth is, and do as we
say ;-)


> @@ -2989,10 +2989,10 @@
>      XSETINT (BVAR (b, display_count), XINT (BVAR (b, display_count)) + 1);
>    BVAR (b, display_time) = Fcurrent_time ();
>  
> -  XSETFASTINT (w->window_end_pos, 0);
> -  XSETFASTINT (w->window_end_vpos, 0);
> +  w->window_end_pos = 0;
> +  w->window_end_vpos = 0;
>    memset (&w->last_cursor, 0, sizeof w->last_cursor);
> -  w->window_end_valid = Qnil;
> +  w->window_end_valid = 0;
>    if (!(keep_margins_p && samebuf))
>      { /* If we're not actually changing the buffer, don't reset hscroll and
>  	 vscroll.  This case happens for example when called from
> @@ -3287,8 +3287,6 @@
>    w->start = Fmake_marker ();
>    w->pointm = Fmake_marker ();
>    w->vertical_scroll_bar_type = Qt;
> -  XSETFASTINT (w->window_end_pos, 0);
> -  XSETFASTINT (w->window_end_vpos, 0);
>  
>    /* Initialize non-Lisp data.  Note that allocate_window zeroes out all
>       non-Lisp data, so do it only for slots which should not be zero.  */
> @@ -3296,6 +3294,8 @@
>    w->phys_cursor_type = -1;
>    w->phys_cursor_width = -1;
>    w->sequence_number = ++sequence_number;
> +  w->window_end_pos = -1;
> +  w->window_end_vpos = -1;

If the "invalid" value is going to be -1, then why did you initialize
with zero above?

> +    /* Z - the buffer position of the last glyph in the current matrix of W.
> +       Should be nonnegative, and valid only if window_end_valid is not zero.  */
> +    ptrdiff_t window_end_pos;

But since you initialize with -1, the "nonnegative" part is only true
when window_end_valid is non-zero, right?

> +    /* Glyph matrix row of the last glyph in the current matrix of W.
> +       Should be nonnegative, and valid only if window_end_valid is not zero.  */
> +    ptrdiff_t window_end_vpos;

Same here.

> @@ -27410,7 +27419,7 @@
>       And verify the buffer's text has not changed.  */
>    b = XBUFFER (w->buffer);
>    if (part == ON_TEXT
> -      && EQ (w->window_end_valid, w->buffer)
> +      && w->window_end_valid

Bother: doesn't the comparison with w->buffer test for a specific
buffer, rather than just non-nil value?  The corresponding assignment,
viz.:

>    if (accurate_p)
>      {
> -      w->window_end_valid = w->buffer;
> +      w->window_end_valid = 1;
>        w->update_mode_line = 0;
>      }

seems to handle the case when the display is "accurate".  Perhaps use
a different value, like 2, for this situation?

Other than that, looks okay, thanks.