Re: [RFC] Getting rid of selected_window

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri, 29 Nov 2013 15:53:46 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> 
> I'm trying to get rid of selected_window, with an intent to completely cleanup corner
> cases where FRAME_SELECTED_WINDOW (selected_frame) is not the same as selected_window
> and avoid redundant synchronization of them.

What for?  I don't think you can get rid of the need to synchronize
those, because of features like focus redirection and minibuffer-only
frames.

> Comments are welcome.

Here are some:

> === modified file 'src/cmds.c'
> --- src/cmds.c	2013-09-05 02:27:13 +0000
> +++ src/cmds.c	2013-11-29 11:02:47 +0000
> @@ -282,7 +282,7 @@
>      nonundocount = 0;
>  
>    if (NILP (Vexecuting_kbd_macro)
> -      && !EQ (minibuf_window, selected_window))
> +      && !EQ (minibuf_window, Fselected_window ()))
>      {
>        if (nonundocount <= 0 || nonundocount >= 20)
>  	{
> 
> === modified file 'src/dispextern.h'
> --- src/dispextern.h	2013-11-06 00:14:56 +0000
> +++ src/dispextern.h	2013-11-29 11:02:47 +0000
> @@ -1410,7 +1410,7 @@
>  
>  #define CURRENT_MODE_LINE_FACE_ID_3(SELW, MBW, SCRW)		\
>       ((!mode_line_in_non_selected_windows			\
> -       || (SELW) == XWINDOW (selected_window)			\
> +       || (SELW) == SELECTED_WINDOW ()				\

I don't understand what is the difference between Fselected_window and
SELECTED_WINDOW.  Why do we use both in C?  It's confusing.

> --- src/editfns.c	2013-11-29 01:22:40 +0000
> +++ src/editfns.c	2013-11-29 11:02:47 +0000
> @@ -832,8 +832,8 @@
>        ? Fcopy_marker (BVAR (current_buffer, mark), Qnil)
>        : Qnil),
>       /* Selected window if current buffer is shown in it, nil otherwise.  */
> -     (EQ (XWINDOW (selected_window)->contents, Fcurrent_buffer ())
> -      ? selected_window : Qnil),
> +     (EQ (SELECTED_WINDOW ()->contents, Fcurrent_buffer ())
> +      ? Fselected_window () : Qnil),

SELECTED_WINDOW can return NULL, and then this dereference will
segfault.

> --- src/fileio.c	2013-11-27 16:08:53 +0000
> +++ src/fileio.c	2013-11-29 11:13:44 +0000
> @@ -3868,8 +3868,8 @@
>  
>  	  /* If display currently starts at beginning of line,
>  	     keep it that way.  */
> -	  if (XBUFFER (XWINDOW (selected_window)->contents) == current_buffer)
> -	    XWINDOW (selected_window)->start_at_line_beg = !NILP (Fbolp ());
> +	  if (SELECTED_BUFFER () == current_buffer)
> +	    SELECTED_WINDOW ()->start_at_line_beg = !NILP (Fbolp ());

Same here (and elsewhere in the patch).

> +#define SELECTED_WINDOW()						\
> +  ((FRAMEP (selected_frame) && FRAME_LIVE_P (XFRAME (selected_frame)))	\
> +   ? XWINDOW (FRAME_SELECTED_WINDOW (XFRAME (selected_frame))) : NULL)

This changes the semantics of selected_window, because it requires
that its frame be live.  I don't think we understand the effects of
this change.

> +/* This is the buffer displayed by the window above.
> +   Note that this is not the same as current_buffer.  */
> +
> +#define SELECTED_BUFFER()						\
> +  (eassert (SELECTED_WINDOW ()), XBUFFER (SELECTED_WINDOW ()->contents))

So now we have SELECTED_BUFFER and current_buffer, which are
different in unspecified ways.  What do you think is the probability
that someone will use the wrong one of that pair?  I think it's 100%.

>    /* do_pending_window_change could change the selected_window due to
>       frame resizing which makes the selected window too small.  */
> -  if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
> +  if ((w = SELECTED_WINDOW ()) != sw)
>      sw = w;

What if SELECTED_WINDOW returns NULL?  You have now assigned NULL to
sw.  The old code did better.

> @@ -13117,7 +13106,7 @@
>  	  && minibuf_level == 0
>  	  /* If the mini-window is currently selected, this means the
>  	     echo-area doesn't show through.  */
> -	  && !MINI_WINDOW_P (XWINDOW (selected_window))))
> +	  && !MINI_WINDOW_P (SELECTED_WINDOW ())))

MINI_WINDOW_P will segfault if SELECTED_WINDOW returns NULL.

> @@ -18115,11 +18101,10 @@
>  GLYPH > 1 or omitted means dump glyphs in long form.  */)
>    (Lisp_Object row, Lisp_Object glyphs)
>  {
> -  struct glyph_matrix *matrix;
> +  struct glyph_matrix *matrix = SELECTED_WINDOW ()->current_matrix;
>    EMACS_INT vpos;
>  
>    CHECK_NUMBER (row);
> -  matrix = XWINDOW (selected_window)->current_matrix;

This will segfault where the old code didn't (if 'row' was not a
number).