Re: [RFC] some reworking of struct window

[Dmitry Antipov]

On 03/21/2013 10:21 PM, Eli Zaretskii wrote:

> > @@ -838,16 +838,8 @@
> >   {
> >     while (w)
> >       {
> > -      if (!NILP (w->hchild))
> > -       {
> > -         eassert (WINDOWP (w->hchild));
> > -         clear_window_matrices (XWINDOW (w->hchild), desired_p);
> > -       }
> > -      else if (!NILP (w->vchild))
> > -       {
> > -         eassert (WINDOWP (w->vchild));
> > -         clear_window_matrices (XWINDOW (w->vchild), desired_p);
> > -       }
> > +      if (WINDOWP (w->object))
> > +       clear_window_matrices (XWINDOW (w->object), desired_p);
> >         else
> >         {
> >           if (desired_p)
>
> Here, you effectively lost the assertion that w->object can only be a
> window or a buffer.  With the new code, if it's neither a window nor a
> buffer, you will behave as if it were a buffer, without checking.

No, because clear_window_matrices doesn't check whether w->buffer is not
nil and call clear_glyph_matrix anyway.

I assume that the most of display-related functions should not be called
for the deleted windows. E.g. this code just silences possible error:

if (WINDOWP (w->object))
  foo (w->object);
else if (BUFFERP (w->object))
  bar (w->object);

The following is better since XBUFFER implies eassert if --enable-checking:

if (WINDOWP (w->object))
  foo (w->object);
else
  bar (XBUFFER (w->object));

Or:

if (WINDOWP (w->object))
  foo (w->object);
else if (BUFFERP (w->object))
  bar (w->object);
else
  emacs_abort ();

The latter example leaves the conditional call to emacs_abort even without
--enable-checking. I suspect that this is too paranoid, and would prefer:

eassert (!NILP (w->object));
if (WINDOWP (w->object))
  foo (w->object);
else
  bar (w->object);

> > -  eassert (NILP (w->hchild) && NILP (w->vchild));
> > +  eassert (w->type == WINDOW_LEAF);
>
> Why do you need to store the WINDOW_LEAF type explicitly, if you can
> test w->object for being a buffer?

Since sync_window_with_frame_matrix_rows assumes live window,
eassert (BUFFERP (w->object)) may be better; but...

> And if you do need this type, why
> not use it instead of WINDOWP in the various tests above and below?

this is not always the same because w->type == WINDOW_xxx_COMBINATION
may be true for the deleted window too.

> > @@ -2069,22 +2060,18 @@
> >     if (!NILP (parent) && NILP (w->combination_limit))
> >       {
> >         p = XWINDOW (parent);
> > -      if (((!NILP (p->vchild) && !NILP (w->vchild))
> > -          || (!NILP (p->hchild) && !NILP (w->hchild))))
> > +      if (p->type == w->type && p->type > WINDOW_LEAF)
>                                     ^^^^^^^^^^^^^^^^^^^^^
> I think you are not supposed to compare enumerated types, except for
> equality.  How exactly the compiler assigns numerical values to
> enumerated types is implementation-defined, I think.

No.

"An enumerator with = defines its enumeration constant as the value of the 
constant
expression. If the first enumerator has no =, the value of its enumeration 
constant
is 0. Each subsequent enumerator with no = defines its enumeration constant as 
the
value of the constant expression obtained by adding 1 to the value of the 
previous
enumeration constant".

This is from the latest C99 draft 
(http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
6.7.2.2 "Enumeration specifiers"), but I believe it was so since long time ago.

> > -  /* P's buffer slot may change from nil to a buffer.  */
> > -  adjust_window_count (p, 1);
>
> Why did you remove this call?

Because I also remove wset_buffer (p, Qnil) few lines below, and per-buffer 
window counters
should be balanced.

> > +/* Window types, see the comment above.  */
> > +
> > +enum window_type
> > +  {
> > +    WINDOW_LEAF,
> > +    WINDOW_HORIZONTAL_COMBINATION,
> > +    WINDOW_VERTICAL_COMBINATION,
> > +  };
>
> The test BUFFERP (w->object) already gives you the same info as
> WINDOW_LEAF type, no?

Of course, it's possible to avoid enum window_type at all and use something 
like:

unsigned combination : 1;
unsigned horizontal : 1;

in struct window. Then we will have

w->type == WINDOW_LEAF equal to !w->combination;
w->type == WINDOW_VERTICAL_COMBINATION equal to w->combination && 
!w->horizontal;
w->type == WINDOW_HORIZONTAL_COMBINATION equal to w->combination && 
w->horizontal.

I'll try this design too.

Dmitry