Re: [RFC] some reworking of struct window

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri, 22 Mar 2013 10:13:42 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> 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.

Sorry, I'm not following your reasoning.

> 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);

But the latter is only protected against nil objects, while the code
only handles windows and buffers.  Any non-nil Lisp object that is
neither a window nor a buffer will slip the assertion.

If there's an immediate XBUFFER in the 'else' branch, then indeed an
assertion is not needed.  But in many cases, this one included,
there's no such call to XBUFFER at all, or it is much later in the
control flow, which makes it harder to find such bugs.

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

OK, so what happens if the enumeration is reshuffled at some later
point in time?  And why did you need to use > anyway, when != would do
the same, and be free of the problem altogether?

> >> -  /* 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.

Sorry, you lost me here.