Re: [RFC] some reworking of struct window

[Eli Zaretskii <eliz@gnu.org>]

> Date: Thu, 21 Mar 2013 13:39:27 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> 
> This patch implements the further generalization of an idea suggested by Stefan
> at http://lists.gnu.org/archive/html/emacs-devel/2013-02/msg00088.html. The main
> motivations are 1) make 'struct window' even bit smaller and 2) simplify the code
> like:
> 
> if (!NILP (w->hchild))
>    foo (w->hchild);
> else if (!NILP (w->vchild))
>    foo (w->vchild);
> else /* assume leaf window with w->buffer */
>    bar (w->buffer);
> 
> to:
> 
> if (WINDOWP (w->object))
>    foo (w->object);
> else /* assume leaf window with buffer at w->object */
>    bar (w->object);

Thanks.

> As usual, reviews and comments are highly appreciated, especially for the
> 'struct window' member which replaces hchild/vchild/buffer (I don't like
> too generic 'object', but couldn't think up the better name).

Most of the changes are mechanical, so the review could be much more
efficient if you could point out non-trivial ones.

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

> -  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?  And if you do need this type, why
not use it instead of WINDOWP in the various tests above and below?

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

> -  /* P's buffer slot may change from nil to a buffer.  */
> -  adjust_window_count (p, 1);

Why did you remove this call?

> -      if (!NILP (w->vchild))
> -	{
> -	  delete_all_child_windows (w->vchild);
> -	  wset_vchild (w, Qnil);
> -	}
> -      else if (!NILP (w->hchild))
> -	{
> -	  delete_all_child_windows (w->hchild);
> -	  wset_hchild (w, Qnil);
> -	}
> -      else if (!NILP (w->buffer))
> +      if (WINDOWP (w->object))
> +	{
> +	  delete_all_child_windows (w->object);
> +	  w->object = Qnil;
> +	}
> +      else
>  	{
>  	  unshow_buffer (w);
>  	  unchain_marker (XMARKER (w->pointm));

Maybe instead of just "else" you should make sure w->object is a
buffer.

> +/* 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?