Re: 'struct window' cleanup #2

[Eli Zaretskii <eliz@gnu.org>]

> Date: Mon, 25 Jun 2012 12:56:22 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> 
> -  w->temslot = w->last_modified = w->last_overlay_modified = Qnil;
> -  XSETFASTINT (w->last_point, 0);
> +  w->temslot = Qnil;
> +  w->last_modified = w->last_overlay_modified = 0;

last_modified and last_overlay_modified could previously be nil as
well as zero.  Now they can only be zero.  I wonder whether this loses
something, like the possibility to distinguish between "no changes"
and "not yet set".

> +    /* Displayed buffer's text modification events counter as of last time
> +       display completed.  */
> +    EMACS_INT last_modified;
> +
> +    /* Displayed buffer's overlays modification events counter as of last
> +       complete update.  */
> +    EMACS_INT last_overlay_modified;
> +
> +    /* Value of point at that time.  */
> +    ptrdiff_t last_point;

Whenever you make some change that touches most or all of the uses of
a certain variable or struct member, please make a point of
documenting the meaning of all of its possible non-trivial values.
For example, in this case, what does it mean for last_point to be
zero? can it ever be negative, and if so, what does that mean? and
similarly for the other two members.  Otherwise, just reading the
comment, one tends to assume that e.g. last_point can only be 1 or
more (as these are the only valid values of point), and introduce
bugs through that incorrect belief.

I had my share of such bugs while hacking the display engine, and as
result came to the conclusion that we must eradicate these problems as
much and as fast as we can, if we want the internals to be reasonably
maintainable.

> @@ -14954,8 +14952,6 @@
>  	   && !NILP (BVAR (current_buffer, mark_active)))
>        && NILP (w->region_showing)
>        && NILP (Vshow_trailing_whitespace)
> -      /* Right after splitting windows, last_point may be nil.  */
> -      && INTEGERP (w->last_point)

I don't think it's correct to remove this test without replacing it
with its equivalent (comparison with zero, I presume).  The value of
point cannot legitimately be zero, but if you leave out the test, the
zero value last_point gets when it's initialized can sneak into the
code below, which doesn't expect such invalid values.  E.g., this
snippet right below:

>  
> -	  if (PT > XFASTINT (w->last_point))
> +	  if (PT > w->last_point)
>  	    {
>  	      /* Point has moved forward.  */
>  	      while (MATRIX_ROW_END_CHARPOS (row) < PT

will now wrongly conclude it should scroll even if point is at
position 1.

Thanks.