'struct window' cleanup #2

'struct window' cleanup #2

[Dmitry Antipov]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

For a `struct window', changing the type of `last_point', `last_modified'
and `last_overlay_modified' from Lisp_Object to appropriate integral
value eliminates a lot of useless Lisp_Object <-> {EMACS_INT, ptrdiff_t}
conversions in a window and redisplay code; and, of course, lesser Lisp_Objects
means a bit faster GC. OK to install?

Dmitry

[-- Attachment #2: window_cleanup.patch --]
[-- Type: text/plain, Size: 16926 bytes --]

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2012-06-24 16:18:41 +0000
+++ admin/ChangeLog	2012-06-25 08:40:47 +0000
@@ -1,3 +1,10 @@
+2012-06-25  Dmitry Antipov  <dmantipov@yandex.ru>
+
+	* coccinelle/window_members.cocci: Semantic patch to adjust all
+	users after changing `last_modified' and `last_overlay_modified'
+	from Lisp_Object to EMACS_INT, and `last_point' from Lisp_Object
+	to ptrdiff_t in a `struct window'.
+
 2012-06-24  Dmitry Antipov  <dmantipov@yandex.ru>
 
 	First Coccinelle semantic patch.

=== added file 'admin/coccinelle/window_members.cocci'
--- admin/coccinelle/window_members.cocci	1970-01-01 00:00:00 +0000
+++ admin/coccinelle/window_members.cocci	2012-06-25 08:40:47 +0000
@@ -0,0 +1,44 @@
+// Adjust all users after changing `last_modified' and `last_overlay_modified'
+// from Lisp_Object to EMACS_INT, and `last_point' from Lisp_Object to 
+// ptrdiff_t in a `struct window'.
+@@
+expression E;
+identifier W;
+@@
+(
+- W->last_point = make_numer (E)
++ W->last_point = E
+|
+- XSETFASTINT (W->last_point, E)
++ W->last_point = E
+|
+- XINT (W->last_point)
++ W->last_point
+|
+- XFASTINT (W->last_point)
++ W->last_point
+|
+- XSETFASTINT (XWINDOW (W)->last_modified, E)
++ XWINDOW (W)->last_modified = E
+|
+- XSETFASTINT (W->last_modified, E)
++ W->last_modified = E
+|
+- W->last_modified = make_number (E)
++ W->last_modified = E
+|
+- XFASTINT (W->last_modified)
++ W->last_modified
+|
+- XSETFASTINT (XWINDOW (W)->last_overlay_modified, E)
++ XWINDOW (W)->last_overlay_modified = E
+|
+- XSETFASTINT (W->last_overlay_modified, E)
++ W->last_overlay_modified = E
+|
+- W->last_overlay_modifed = make_number (E)
++ W->last_overlay_modifed = E
+|
+- XFASTINT (W->last_overlay_modified)
++ W->last_overlay_modified
+)

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-06-25 07:54:45 +0000
+++ src/ChangeLog	2012-06-25 08:42:44 +0000
@@ -1,3 +1,10 @@
+2012-06-25  Dmitry Antipov  <dmantipov@yandex.ru>
+
+	* window.h (struct window): For a `struct window', change
+	`last_modified' and `last_overlay_modified' from Lisp_Object
+	to EMACS_INT, and `last_point' from Lisp_Object to ptrdiff_t.
+	Adjust users accordingly.
+
 2012-06-25  Paul Eggert  <eggert@cs.ucla.edu>
 
 	* gtkutil.c (get_utf8_string): Remove redundant assignment.

=== modified file 'src/frame.c'
--- src/frame.c	2012-06-19 06:49:50 +0000
+++ src/frame.c	2012-06-25 08:40:47 +0000
@@ -131,7 +131,7 @@
 {
   struct window *w = XWINDOW (window);
 
-  XSETFASTINT (w->last_modified, 0);
+  w->last_modified = 0;
   XSETFASTINT (w->top_line, XFASTINT (w->top_line) + n);
   XSETFASTINT (w->total_lines, XFASTINT (w->total_lines) - n);
 

=== modified file 'src/minibuf.c'
--- src/minibuf.c	2012-06-19 17:05:41 +0000
+++ src/minibuf.c	2012-06-25 08:40:47 +0000
@@ -888,8 +888,8 @@
 
   /* Make sure minibuffer window is erased, not ignored.  */
   windows_or_buffers_changed++;
-  XSETFASTINT (XWINDOW (window)->last_modified, 0);
-  XSETFASTINT (XWINDOW (window)->last_overlay_modified, 0);
+  XWINDOW (window)->last_modified = 0;
+  XWINDOW (window)->last_overlay_modified = 0;
 
   /* In case the previous minibuffer displayed in this miniwindow is
      dead, we may keep displaying this buffer (tho it's inactive), so reset it,

=== modified file 'src/window.c'
--- src/window.c	2012-06-19 16:56:28 +0000
+++ src/window.c	2012-06-25 08:40:47 +0000
@@ -1314,8 +1314,8 @@
 
   if (! NILP (update)
       && ! (! NILP (w->window_end_valid)
-	    && XFASTINT (w->last_modified) >= BUF_MODIFF (b)
-	    && XFASTINT (w->last_overlay_modified) >= BUF_OVERLAY_MODIFF (b))
+	    && w->last_modified >= BUF_MODIFF (b)
+	    && w->last_overlay_modified >= BUF_OVERLAY_MODIFF (b))
       && !noninteractive)
     {
       struct text_pos startp;
@@ -1398,8 +1398,8 @@
   if (NILP (noforce))
     w->force_start = 1;
   w->update_mode_line = 1;
-  XSETFASTINT (w->last_modified, 0);
-  XSETFASTINT (w->last_overlay_modified, 0);
+  w->last_modified = 0;
+  w->last_overlay_modified = 0;
   if (!EQ (window, selected_window))
     windows_or_buffers_changed++;
 
@@ -1511,8 +1511,8 @@
   if (NILP (w->window_end_valid)
       || current_buffer->clip_changed
       || current_buffer->prevent_redisplay_optimizations_p
-      || XFASTINT (w->last_modified) < BUF_MODIFF (b)
-      || XFASTINT (w->last_overlay_modified) < BUF_OVERLAY_MODIFF (b))
+      || w->last_modified < BUF_MODIFF (b)
+      || w->last_overlay_modified < BUF_OVERLAY_MODIFF (b))
     return Qnil;
 
   if (NILP (line))
@@ -3011,8 +3011,8 @@
 			     buffer);
       w->start_at_line_beg = 0;
       w->force_start = 0;
-      XSETFASTINT (w->last_modified, 0);
-      XSETFASTINT (w->last_overlay_modified, 0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
     }
   /* Maybe we could move this into the `if' but it's not obviously safe and
      I doubt it's worth the trouble.  */
@@ -3298,8 +3298,9 @@
   XSETFASTINT (w->use_time, 0);
   ++sequence_number;
   XSETFASTINT (w->sequence_number, sequence_number);
-  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;
+  w->last_point = 0;
   w->last_had_star = 0;
   w->vertical_scroll_bar = Qnil;
   w->left_margin_cols = w->right_margin_cols = Qnil;
@@ -3518,8 +3519,8 @@
     }
 
   /* Clear out some redisplay caches.  */
-  XSETFASTINT (w->last_modified, 0);
-  XSETFASTINT (w->last_overlay_modified, 0);
+  w->last_modified = 0;
+  w->last_overlay_modified = 0;
 }
 
 
@@ -4051,8 +4052,8 @@
       /* Grow the mini-window.  */
       XSETFASTINT (w->top_line, XFASTINT (r->top_line) + XFASTINT (r->total_lines));
       XSETFASTINT (w->total_lines, XFASTINT (w->total_lines) - XINT (value));
-      XSETFASTINT (w->last_modified, 0);
-      XSETFASTINT (w->last_overlay_modified, 0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
 
       adjust_glyphs (f);
       UNBLOCK_INPUT;
@@ -4087,8 +4088,8 @@
 	  XSETFASTINT (w->top_line, XFASTINT (r->top_line) + XFASTINT (r->total_lines));
 	  XSETFASTINT (w->total_lines, 1);
 
-	  XSETFASTINT (w->last_modified, 0);
-	  XSETFASTINT (w->last_overlay_modified, 0);
+	  w->last_modified = 0;
+	  w->last_overlay_modified = 0;
 
 	  adjust_glyphs (f);
 	  UNBLOCK_INPUT;
@@ -4315,8 +4316,8 @@
 					 w->buffer);
 		  w->start_at_line_beg = 1;
 		  w->update_mode_line = 1;
-		  XSETFASTINT (w->last_modified, 0);
-		  XSETFASTINT (w->last_overlay_modified, 0);
+		  w->last_modified = 0;
+		  w->last_overlay_modified = 0;
 		  /* Set force_start so that redisplay_window will run the
 		     window-scroll-functions.  */
 		  w->force_start = 1;
@@ -4461,8 +4462,8 @@
       bytepos = XMARKER (w->start)->bytepos;
       w->start_at_line_beg = (pos == BEGV || FETCH_BYTE (bytepos - 1) == '\n');
       w->update_mode_line = 1;
-      XSETFASTINT (w->last_modified, 0);
-      XSETFASTINT (w->last_overlay_modified, 0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
       /* Set force_start so that redisplay_window will run the
 	 window-scroll-functions.  */
       w->force_start = 1;
@@ -4660,8 +4661,8 @@
       set_marker_restricted_both (w->start, w->buffer, pos, pos_byte);
       w->start_at_line_beg = !NILP (bolp);
       w->update_mode_line = 1;
-      XSETFASTINT (w->last_modified, 0);
-      XSETFASTINT (w->last_overlay_modified, 0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
       /* Set force_start so that redisplay_window will run
 	 the window-scroll-functions.  */
       w->force_start = 1;
@@ -5601,8 +5602,8 @@
 		}
 	    }
 
-	  XSETFASTINT (w->last_modified, 0);
-	  XSETFASTINT (w->last_overlay_modified, 0);
+	  w->last_modified = 0;
+	  w->last_overlay_modified = 0;
 
 	  /* Reinstall the saved buffer and pointers into it.  */
 	  if (NILP (p->buffer))

=== modified file 'src/window.h'
--- src/window.h	2012-06-01 03:41:03 +0000
+++ src/window.h	2012-06-25 08:40:47 +0000
@@ -157,14 +157,6 @@
        bookkeeping.  */
     Lisp_Object temslot;
 
-    /* text.modified of displayed buffer as of last time display
-       completed.  */
-    Lisp_Object last_modified;
-    /* BUF_OVERLAY_MODIFIED of displayed buffer as of last complete update.  */
-    Lisp_Object last_overlay_modified;
-    /* Value of point at that time.  */
-    Lisp_Object last_point;
-
     /* This window's vertical scroll bar.  This field is only for use
        by the window-system-dependent code which implements the
        scroll bars; it can store anything it likes here.  If this
@@ -254,6 +246,17 @@
     struct glyph_matrix *current_matrix;
     struct glyph_matrix *desired_matrix;
 
+    /* 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;
+
     /* Scaling factor for the glyph_matrix size calculation in this window.
        Used if window contains many small images or uses proportional fonts,
        as the normal  may yield a matrix which is too small.  */

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2012-06-22 21:17:42 +0000
+++ src/xdisp.c	2012-06-25 08:40:47 +0000
@@ -12510,8 +12510,8 @@
   int unchanged_p = 1;
 
   /* If text or overlays have changed, see where.  */
-  if (XFASTINT (w->last_modified) < MODIFF
-      || XFASTINT (w->last_overlay_modified) < OVERLAY_MODIFF)
+  if (w->last_modified < MODIFF
+      || w->last_overlay_modified < OVERLAY_MODIFF)
     {
       /* Gap in the line?  */
       if (GPT < start || Z - GPT < end)
@@ -12790,9 +12790,9 @@
 	pt = marker_position (w->pointm);
 
       if ((w->current_matrix->buffer != XBUFFER (w->buffer)
-	   || pt != XINT (w->last_point))
+	   || pt != w->last_point)
 	  && check_point_in_composition (w->current_matrix->buffer,
-					 XINT (w->last_point),
+					 w->last_point,
 					 XBUFFER (w->buffer), pt))
 	b->clip_changed = 1;
     }
@@ -13014,9 +13014,9 @@
   if (!NILP (w->column_number_displayed)
       /* This alternative quickly identifies a common case
 	 where no change is needed.  */
-      && !(PT == XFASTINT (w->last_point)
-	   && XFASTINT (w->last_modified) >= MODIFF
-	   && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF)
+      && !(PT == w->last_point
+	   && w->last_modified >= MODIFF
+	   && w->last_overlay_modified >= OVERLAY_MODIFF)
       && (XFASTINT (w->column_number_displayed) != current_column ()))
     w->update_mode_line = 1;
 
@@ -13079,8 +13079,8 @@
     }
   else if (EQ (selected_window, minibuf_window)
 	   && (current_buffer->clip_changed
-	       || XFASTINT (w->last_modified) < MODIFF
-	       || XFASTINT (w->last_overlay_modified) < OVERLAY_MODIFF)
+	       || w->last_modified < MODIFF
+	       || w->last_overlay_modified < OVERLAY_MODIFF)
 	   && resize_mini_window (w, 0))
     {
       /* Resized active mini-window to fit the size of what it is
@@ -13145,8 +13145,8 @@
 	      || FETCH_BYTE (BYTEPOS (tlbufpos)) == '\n'))
 	/* Former continuation line has disappeared by becoming empty.  */
 	goto cancel;
-      else if (XFASTINT (w->last_modified) < MODIFF
-	       || XFASTINT (w->last_overlay_modified) < OVERLAY_MODIFF
+      else if (w->last_modified < MODIFF
+	       || w->last_overlay_modified < OVERLAY_MODIFF
 	       || MINI_WINDOW_P (w))
 	{
 	  /* We have to handle the case of continuation around a
@@ -13250,7 +13250,7 @@
 	    goto cancel;
 	}
       else if (/* Cursor position hasn't changed.  */
-	       PT == XFASTINT (w->last_point)
+	       PT == w->last_point
 	       /* Make sure the cursor was last displayed
 		  in this window.  Otherwise we have to reposition it.  */
 	       && 0 <= w->cursor.vpos
@@ -13666,10 +13666,8 @@
     {
       struct buffer *b = XBUFFER (w->buffer);
 
-      w->last_modified
-	= make_number (accurate_p ? BUF_MODIFF (b) : 0);
-      w->last_overlay_modified
-	= make_number (accurate_p ? BUF_OVERLAY_MODIFF (b) : 0);
+      w->last_modified = accurate_p ? BUF_MODIFF(b) : 0;
+      w->last_overlay_modified = accurate_p ? BUF_OVERLAY_MODIFF (b) : 0;
       w->last_had_star
 	= BUF_MODIFF (b) > BUF_SAVE_MODIFF (b);
 
@@ -13691,9 +13689,9 @@
 	  w->last_cursor_off_p = w->cursor_off_p;
 
 	  if (w == XWINDOW (selected_window))
-	    w->last_point = make_number (BUF_PT (b));
+	    w->last_point = BUF_PT (b);
 	  else
-	    w->last_point = make_number (XMARKER (w->pointm)->charpos);
+	    w->last_point = XMARKER (w->pointm)->charpos;
 	}
     }
 
@@ -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)
       /* This code is not used for mini-buffer for the sake of the case
 	 of redisplaying to replace an echo area message; since in
 	 that case the mini-buffer contents per se are usually
@@ -15013,7 +15009,7 @@
 	  int scroll_p = 0, must_scroll = 0;
 	  int last_y = window_text_bottom_y (w) - this_scroll_margin;
 
-	  if (PT > XFASTINT (w->last_point))
+	  if (PT > w->last_point)
 	    {
 	      /* Point has moved forward.  */
 	      while (MATRIX_ROW_END_CHARPOS (row) < PT
@@ -15048,7 +15044,7 @@
 		      && !MATRIX_ROW_ENDS_IN_MIDDLE_OF_CHAR_P (row)))
 		scroll_p = 1;
 	    }
-	  else if (PT < XFASTINT (w->last_point))
+	  else if (PT < w->last_point)
 	    {
 	      /* Cursor has to be moved backward.  Note that PT >=
 		 CHARPOS (startp) because of the outer if-statement.  */
@@ -15394,8 +15390,8 @@
     = (!NILP (w->window_end_valid)
        && !current_buffer->clip_changed
        && !current_buffer->prevent_redisplay_optimizations_p
-       && XFASTINT (w->last_modified) >= MODIFF
-       && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF);
+       && w->last_modified >= MODIFF
+       && w->last_overlay_modified >= OVERLAY_MODIFF);
 
   /* Run the window-bottom-change-functions
      if it is possible that the text on the screen has changed
@@ -15417,8 +15413,8 @@
   buffer_unchanged_p
     = (!NILP (w->window_end_valid)
        && !current_buffer->clip_changed
-       && XFASTINT (w->last_modified) >= MODIFF
-       && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF);
+       && w->last_modified >= MODIFF
+       && w->last_overlay_modified >= OVERLAY_MODIFF);
 
   /* When windows_or_buffers_changed is non-zero, we can't rely on
      the window end being valid, so set it to nil there.  */
@@ -15443,9 +15439,9 @@
   if (!NILP (w->column_number_displayed)
       /* This alternative quickly identifies a common case
 	 where no change is needed.  */
-      && !(PT == XFASTINT (w->last_point)
-	   && XFASTINT (w->last_modified) >= MODIFF
-	   && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF)
+      && !(PT == w->last_point
+	   && w->last_modified >= MODIFF
+	   && w->last_overlay_modified >= OVERLAY_MODIFF)
       && (XFASTINT (w->column_number_displayed) != current_column ()))
     update_mode_line = 1;
 
@@ -15561,8 +15557,8 @@
 	  startp = run_window_scroll_functions (window, startp);
 	}
 
-      w->last_modified = make_number (0);
-      w->last_overlay_modified = make_number (0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
       if (CHARPOS (startp) < BEGV)
 	SET_TEXT_POS (startp, BEGV, BEGV_BYTE);
       else if (CHARPOS (startp) > ZV)
@@ -15687,8 +15683,8 @@
 	   && (CHARPOS (startp) < ZV
 	       /* Avoid starting at end of buffer.  */
 	       || CHARPOS (startp) == BEGV
-	       || (XFASTINT (w->last_modified) >= MODIFF
-		   && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF)))
+	       || (w->last_modified >= MODIFF
+		   && w->last_overlay_modified >= OVERLAY_MODIFF)))
     {
       int d1, d2, d3, d4, d5, d6;
 
@@ -15775,8 +15771,8 @@
 
  try_to_scroll:
 
-  w->last_modified = make_number (0);
-  w->last_overlay_modified = make_number (0);
+  w->last_modified = 0;
+  w->last_overlay_modified = 0;
 
   /* Redisplay the mode line.  Select the buffer properly for that.  */
   if (!update_mode_line)
@@ -17135,7 +17131,7 @@
     GIVE_UP (5);
 
   /* Another way to prevent redisplay optimizations.  */
-  if (XFASTINT (w->last_modified) == 0)
+  if (w->last_modified == 0)
     GIVE_UP (6);
 
   /* Verify that window is not hscrolled.  */
@@ -27412,8 +27408,8 @@
   b = XBUFFER (w->buffer);
   if (part == ON_TEXT
       && EQ (w->window_end_valid, w->buffer)
-      && XFASTINT (w->last_modified) == BUF_MODIFF (b)
-      && XFASTINT (w->last_overlay_modified) == BUF_OVERLAY_MODIFF (b))
+      && w->last_modified == BUF_MODIFF (b)
+      && w->last_overlay_modified == BUF_OVERLAY_MODIFF (b))
     {
       int hpos, vpos, dx, dy, area = LAST_AREA;
       ptrdiff_t pos;

Re: 'struct window' cleanup #2

[John Wiegley]

>>>>> Dmitry Antipov <dmantipov@yandex.ru> writes:

> For a `struct window', changing the type of `last_point', `last_modified'
> and `last_overlay_modified' from Lisp_Object to appropriate integral value
> eliminates a lot of useless Lisp_Object <-> {EMACS_INT, ptrdiff_t}
> conversions in a window and redisplay code; and, of course, lesser
> Lisp_Objects means a bit faster GC. OK to install?

Dmitry, have you measured what speed/memory advantage this change represents?
I understand the theory of why it may be valuable, but what has convinced you
that it's a hotspot?

John

Re: 'struct window' cleanup #2

[Paul Eggert]

I like this change, as it simplifies the code and makes
it easier to maintain.  It also no doubt shrinks and
speeds up the code a bit, which is a plus too.  I
don't think it's necessary to go to much effort to
measure the performance improvement.

Re: 'struct window' cleanup #2

[Dmitry Antipov]

On 06/25/2012 06:22 PM, John Wiegley wrote:

> Dmitry, have you measured what speed/memory advantage this change represents?
> I understand the theory of why it may be valuable, but what has convinced you
> that it's a hotspot?

1. The main bottleneck of Emacs' C code is mark_object; profiling says it may
consume 25-30% of CPU time for some workloads like long byte-compile runs.
So, lesser calls to mark_object is always desirable.

2. It doesn't make sense to use Lisp_Object for the integer values which are:
   a) invisible from Lisp;
   b) always contains an integer values (no nil or t with special treatment,
      for example);
   c) always converted to C integer values for the sake of comparison.

Dmitry

Re: 'struct window' cleanup #2

[Stefan Monnier]

> For a `struct window', changing the type of `last_point', `last_modified'
> and `last_overlay_modified' from Lisp_Object to appropriate integral
> value eliminates a lot of useless Lisp_Object <-> {EMACS_INT, ptrdiff_t}
> conversions in a window and redisplay code; and, of course, lesser Lisp_Objects
> means a bit faster GC. OK to install?

hscroll and min_hscroll are good candidates as well.


        Stefan

Re: 'struct window' cleanup #2

[Dmitry Antipov]

On 06/25/2012 06:53 PM, Stefan Monnier wrote:
>> For a `struct window', changing the type of `last_point', `last_modified'
>> and `last_overlay_modified' from Lisp_Object to appropriate integral
>> value eliminates a lot of useless Lisp_Object <-> {EMACS_INT, ptrdiff_t}
>> conversions in a window and redisplay code; and, of course, lesser Lisp_Objects
>> means a bit faster GC. OK to install?
>
> hscroll and min_hscroll are good candidates as well.

IIUC, doing this means that we can't treat saved_window as Lisp_Vector anymore - it
should be one more pseudo-vector type like save_window_data instead. Is it worth doing?

Dmitry

Re: 'struct window' cleanup #2

[martin rudalics]

 > hscroll and min_hscroll are good candidates as well.

How about window_end_pos, window_end_vpos and window_end_valid?

martin

Re: 'struct window' cleanup #2

[Eli Zaretskii]

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

Re: 'struct window' cleanup #2

[Dmitry Antipov]

On 06/25/2012 08:35 PM, martin rudalics wrote:
>  > hscroll and min_hscroll are good candidates as well.
>
> How about window_end_pos, window_end_vpos and window_end_valid?

xdisp.c assigns w->buffer to w->window_end_valid, so it may be tricky.
I suppose window_end_valid may be a bitfield; so, NILP (W->window_end_valid)
becomes !W->window_end_valid, EQ (w->window_end_valid, w->buffer)
becomes W->window_end_valid, etc.

Dmitry

Re: 'struct window' cleanup #2

[martin rudalics]

 >> How about window_end_pos, window_end_vpos and window_end_valid?
 >
 > xdisp.c assigns w->buffer to w->window_end_valid, so it may be tricky.
 > I suppose window_end_valid may be a bitfield; so, NILP
 > (W->window_end_valid)
 > becomes !W->window_end_valid, EQ (w->window_end_valid, w->buffer)
 > becomes W->window_end_valid, etc.

Indeed.  Looks like a hack to detect whether the window still shows the
same buffer.  Anyway, window_end_pos and window_end_vpos are the more
promising candidates (if Eli agrees).  Can you see whether the part

       /* When splitting windows or for new windows, it happens that
	 redisplay is called with a nil window_end_vpos or one being
	 larger than the window.  This should really be fixed in
	 window.c.  I don't have this on my list, now, so we do
	 approximately the same as the old redisplay code.  --gerd.  */
       && INTEGERP (w->window_end_vpos)

in xdisp.c still makes sense?  IIUC window_end_vpos is always an
integer.

martin

Re: 'struct window' cleanup #2

[Dmitry Antipov]

On 06/26/2012 11:26 AM, martin rudalics wrote:

> Indeed.  Looks like a hack to detect whether the window still shows the
> same buffer.  Anyway, window_end_pos and window_end_vpos are the more
> promising candidates (if Eli agrees).  Can you see whether the part
>
>        /* When splitting windows or for new windows, it happens that
>       redisplay is called with a nil window_end_vpos or one being
>       larger than the window.  This should really be fixed in
>       window.c.  I don't have this on my list, now, so we do
>       approximately the same as the old redisplay code.  --gerd.  */
>        && INTEGERP (w->window_end_vpos)
>
> in xdisp.c still makes sense?  IIUC window_end_vpos is always an
> integer.

Hmmm... I'll try, although xdisp.c looks like a nightmare
and I haven't yet tried to understand how it works :-).

Dmitry

Re: 'struct window' cleanup #2

[Eli Zaretskii]

> Date: Tue, 26 Jun 2012 09:26:52 +0200
> From: martin rudalics <rudalics@gmx.at>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
> 	Emacs development discussions <emacs-devel@gnu.org>
> 
>  >> How about window_end_pos, window_end_vpos and window_end_valid?
>  >
>  > xdisp.c assigns w->buffer to w->window_end_valid, so it may be tricky.
>  > I suppose window_end_valid may be a bitfield; so, NILP
>  > (W->window_end_valid)
>  > becomes !W->window_end_valid, EQ (w->window_end_valid, w->buffer)
>  > becomes W->window_end_valid, etc.
> 
> Indeed.  Looks like a hack to detect whether the window still shows the
> same buffer.

Why a "hack"?  Lisp object are good precisely for this reason: that
you can give them values of different types of object.

> Anyway, window_end_pos and window_end_vpos are the more
> promising candidates (if Eli agrees).

I don't object.  But if we think that Lisp integers cause any
significant slowdown during GC, we could special-case them in
mark_object, because that is a no-op for integers.  That would be less
work and less potential bugs.

>  Can you see whether the part
> 
>        /* When splitting windows or for new windows, it happens that
> 	 redisplay is called with a nil window_end_vpos or one being
> 	 larger than the window.  This should really be fixed in
> 	 window.c.  I don't have this on my list, now, so we do
> 	 approximately the same as the old redisplay code.  --gerd.  */
>        && INTEGERP (w->window_end_vpos)
> 
> in xdisp.c still makes sense?

No, there's no code anymore that sets it to nil.

Re: 'struct window' cleanup #2

[Eli Zaretskii]

> Date: Tue, 26 Jun 2012 13:06:52 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
> 	Emacs development discussions <emacs-devel@gnu.org>
> 
> On 06/26/2012 11:26 AM, martin rudalics wrote:
> 
> > Indeed.  Looks like a hack to detect whether the window still shows the
> > same buffer.  Anyway, window_end_pos and window_end_vpos are the more
> > promising candidates (if Eli agrees).  Can you see whether the part
> >
> >        /* When splitting windows or for new windows, it happens that
> >       redisplay is called with a nil window_end_vpos or one being
> >       larger than the window.  This should really be fixed in
> >       window.c.  I don't have this on my list, now, so we do
> >       approximately the same as the old redisplay code.  --gerd.  */
> >        && INTEGERP (w->window_end_vpos)
> >
> > in xdisp.c still makes sense?  IIUC window_end_vpos is always an
> > integer.
> 
> Hmmm... I'll try, although xdisp.c looks like a nightmare
> and I haven't yet tried to understand how it works :-).

I'm hacking xdisp.c intensively for 3 years, and I still get surprised
from time to time.

Re: 'struct window' cleanup #2

[Dmitry Antipov]

On 06/26/2012 07:32 PM, Eli Zaretskii wrote:

> I don't object.  But if we think that Lisp integers cause any
> significant slowdown during GC, we could special-case them in
> mark_object, because that is a no-op for integers.  That would be less
> work and less potential bugs.

Hm... mark_object is an absolute bottleneck now, and, IMHO,
handling one more special case (just one conditional branch)
will make it even worse.

Dmitry

Re: 'struct window' cleanup #2

[Eli Zaretskii]

> Date: Tue, 26 Jun 2012 20:49:46 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: martin rudalics <rudalics@gmx.at>, monnier@iro.umontreal.ca, 
>  emacs-devel@gnu.org
> 
> On 06/26/2012 07:32 PM, Eli Zaretskii wrote:
> 
> > I don't object.  But if we think that Lisp integers cause any
> > significant slowdown during GC, we could special-case them in
> > mark_object, because that is a no-op for integers.  That would be less
> > work and less potential bugs.
> 
> Hm... mark_object is an absolute bottleneck now, and, IMHO,
> handling one more special case (just one conditional branch)
> will make it even worse.

Sorry, I meant special-case the integers in mark_vectorlike, which
calls mark_object for Lisp integers.

Re: 'struct window' cleanup #2

[Stefan Monnier]

>> Anyway, window_end_pos and window_end_vpos are the more
>> promising candidates (if Eli agrees).
> I don't object.  But if we think that Lisp integers cause any
> significant slowdown during GC

For me, the reason to make such changes is to make the code simpler and
more robust: by avoiding conversion to/from Lisp_Object we get to use
more of C's type checks (as weak as they are, they're better than
nothing).
And for objects which are always integers, the code ends up simpler.
If it's more efficient, that's just a nice side-effect.


        Stefan

Re: 'struct window' cleanup #2

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: martin rudalics <rudalics@gmx.at>,  dmantipov@yandex.ru,  emacs-devel@gnu.org
> Date: Tue, 26 Jun 2012 20:42:37 -0400
> 
> >> Anyway, window_end_pos and window_end_vpos are the more
> >> promising candidates (if Eli agrees).
> > I don't object.  But if we think that Lisp integers cause any
> > significant slowdown during GC
> 
> For me, the reason to make such changes is to make the code simpler and
> more robust: by avoiding conversion to/from Lisp_Object we get to use
> more of C's type checks (as weak as they are, they're better than
> nothing).
> And for objects which are always integers, the code ends up simpler.
> If it's more efficient, that's just a nice side-effect.

As I said, I don't object.  I was responding to what seemed to be
Dmitry's main motivation: make GC consume less resources.

Re: 'struct window' cleanup #2

[martin rudalics]

 > Why a "hack"?  Lisp object are good precisely for this reason: that
 > you can give them values of different types of object.

window_end_valid is documented as

     /* t if window_end_pos is truly valid.
        This is nil if nontrivial redisplay is preempted
        since in that case the frame image that window_end_pos
        did not get onto the frame.  */
     Lisp_Object window_end_valid;

but the code in xdisp.c checks it usually just as

      !NILP (w->window_end_valid)

Consequently, the assignment

       w->window_end_valid = w->buffer;

looks fragile (one has to be sure that the NILP checks don't happen
while it's set to the buffer since otherwise we end up trying again for
no use) and at least contradicts the initial comment of try_window_id

/* Try to redisplay window W by reusing its existing display.  W's
    current matrix must be up to date when this function is called,
    i.e. window_end_valid must not be nil.

but apparently the current matrix is not up to date when the value is
non-nil but some other buffer.  So unless this is better documented
and/or the corresponding code cleaned up it remains a "hack" IMHO.

 >> Anyway, window_end_pos and window_end_vpos are the more
 >> promising candidates (if Eli agrees).
 >
 > I don't object.  But if we think that Lisp integers cause any
 > significant slowdown during GC, we could special-case them in
 > mark_object, because that is a no-op for integers.  That would be less
 > work and less potential bugs.

I don't care about GC.  But it seems to me that _all_ accesses of these
two structure members are via make_number and XFASTINT.  So we can only
gain from cleaning up this.

 >>  Can you see whether the part
 >>
 >>        /* When splitting windows or for new windows, it happens that
 >> 	 redisplay is called with a nil window_end_vpos or one being
 >> 	 larger than the window.  This should really be fixed in
 >> 	 window.c.  I don't have this on my list, now, so we do
 >> 	 approximately the same as the old redisplay code.  --gerd.  */
 >>        && INTEGERP (w->window_end_vpos)
 >>
 >> in xdisp.c still makes sense?
 >
 > No, there's no code anymore that sets it to nil.

Fine (apparently that was another "hack" in redisplay).

martin

'struct window' cleanup #3

[Dmitry Antipov]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

This patch changes the following slots of `struct window'
from Lisp_Object to:

   use_time -> int
   sequence_number -> int
   last_modified -> EMACS_INT
   last_overlay_modified -> EMACS_INT
   last_point -> ptrdiff_t
   window_end_pos -> ptrdiff_t
   window_end_vpos -> ptrdiff_t

Notes:

1. IIUC, `window_initialized' is redundant and obsolete.

2. Fields hscroll and min_hscroll are good candidates for conversion
    too, but this requires some redesign of `struct saved_window', most
    probably to yet another pseudovector.

Special notes about hard stuff in xdisp.c:

1. Field window_end_valid is used in a tricky way which isn't quite
    clear for me :-), so I would like to not touch it at this time
    (hopefully I will get back to it later).

2. Since last_point is an integer, this check is no longer valid:

    /* Right after splitting windows, last_point may be nil.  */
    && INTEGERP (w->last_point);

    My tests never shows zero here, so I'm installing assertion instead.

3. The check:

    && INTEGERP (w->window_end_vpos)
    && XFASTINT (w->window_end_vpos) < w->current_matrix->nrows

    is transformed to:

    && w->window_end_vpos > 0
    && w->window_end_vpos < w->current_matrix->nrows

    I never get w->window_end_vpos >= w->current_matrix->nrows, but
    run into w->window_end_vpos == 0 several times. At this moment, I have
    no ideas whether Gerd was correct about fixing window.c, so this
    needs more detailed investigations.

Dmitry

[-- Attachment #2: window_cleanup.patch --]
[-- Type: text/plain, Size: 38424 bytes --]

=== added file 'admin/coccinelle/window.cocci'
--- admin/coccinelle/window.cocci	1970-01-01 00:00:00 +0000
+++ admin/coccinelle/window.cocci	2012-06-27 04:53:35 +0000
@@ -0,0 +1,140 @@
+// Make changes needed after converting slots of `struct window'
+// from Lisp_Object to:
+//   - last_point -> ptrdiff_t
+//   - last_modified -> EMACS_INT
+//   - last_overlay_modified -> EMACS_INT
+//   - window_end_pos -> ptrdiff_t
+//   - window_end_vpos -> ptrdiff_t
+@@
+expression E;
+identifier W;
+@@
+(
+- XSETFASTINT (XWINDOW (W)->last_point, E)
++ XWINDOW (W)->last_point = E
+|
+- XSETFASTINT (W->last_point, E)
++ W->last_point = E
+|
+- XFASTINT (XWINDOW (W)->last_point)
++ XWINDOW (W)->last_point
+|
+- XFASTINT (W->last_point)
++ W->last_point
+|
+- XINT (XWINDOW (W)->last_point)
++ XWINDOW (W)->last_point
+|
+- XINT (W->last_point)
++ W->last_point
+|
+- XWINDOW (W)->last_point = make_number (E)
++ XWINDOW (W)->last_point = E
+|
+- W->last_point = make_number (E)
++ W->last_point = E
+
+|
+
+- XSETFASTINT (XWINDOW (W)->last_modified, E)
++ XWINDOW (W)->last_modified = E
+|
+- XSETFASTINT (W->last_modified, E)
++ W->last_modified = E
+|
+- XFASTINT (XWINDOW (W)->last_modified)
++ XWINDOW (W)->last_modified
+|
+- XFASTINT (W->last_modified)
++ W->last_modified
+|
+- XINT (XWINDOW (W)->last_modified)
++ XWINDOW (W)->last_modified
+|
+- XINT (W->last_modified)
++ W->last_modified
+|
+- XWINDOW (W)->last_modified = make_number (E)
++ XWINDOW (W)->last_modified = E
+|
+- W->last_modified = make_number (E)
++ W->last_modified = E
+
+|
+
+- XSETFASTINT (XWINDOW (W)->last_overlay_modified, E)
++ XWINDOW (W)->last_overlay_modified = E
+|
+- XSETFASTINT (W->last_overlay_modified, E)
++ W->last_overlay_modified = E
+|
+- XFASTINT (XWINDOW (W)->last_overlay_modified)
++ XWINDOW (W)->last_overlay_modified
+|
+- XFASTINT (W->last_overlay_modified)
++ W->last_overlay_modified
+|
+- XINT (XWINDOW (W)->last_overlay_modified)
++ XWINDOW (W)->last_overlay_modified
+|
+- XINT (W->last_overlay_modified)
++ W->last_overlay_modified
+|
+- XWINDOW (W)->last_overlay_modified = make_number (E)
++ XWINDOW (W)->last_overlay_modified = E
+|
+- W->last_overlay_modified = make_number (E)
++ W->last_overlay_modified = E
+
+|
+
+- XSETFASTINT (XWINDOW (W)->window_end_pos, E)
++ XWINDOW (W)->window_end_pos = E
+|
+- XSETFASTINT (W->window_end_pos, E)
++ W->window_end_pos = E
+|
+- XFASTINT (XWINDOW (W)->window_end_pos)
++ XWINDOW (W)->window_end_pos
+|
+- XFASTINT (W->window_end_pos)
++ W->window_end_pos
+|
+- XINT (XWINDOW (W)->window_end_pos)
++ XWINDOW (W)->window_end_pos
+|
+- XINT (W->window_end_pos)
++ W->window_end_pos
+|
+- XWINDOW (W)->window_end_pos = make_number (E)
++ XWINDOW (W)->window_end_pos = E
+|
+- W->window_end_pos = make_number (E)
++ W->window_end_pos = E
+
+|
+
+- XSETFASTINT (XWINDOW (W)->window_end_vpos, E)
++ XWINDOW (W)->window_end_vpos = E
+|
+- XSETFASTINT (W->window_end_vpos, E)
++ W->window_end_vpos = E
+|
+- XFASTINT (XWINDOW (W)->window_end_vpos)
++ XWINDOW (W)->window_end_vpos
+|
+- XFASTINT (W->window_end_vpos)
++ W->window_end_vpos
+|
+- XINT (XWINDOW (W)->window_end_vpos)
++ XWINDOW (W)->window_end_vpos
+|
+- XINT (W->window_end_vpos)
++ W->window_end_vpos
+|
+- XWINDOW (W)->window_end_vpos = make_number (E)
++ XWINDOW (W)->window_end_vpos = E
+|
+- W->window_end_vpos = make_number (E)
++ W->window_end_vpos = E
+)

=== modified file 'src/dispnew.c'
--- src/dispnew.c	2012-06-24 04:11:19 +0000
+++ src/dispnew.c	2012-06-27 04:52:47 +0000
@@ -643,8 +643,7 @@
 
 	      /* Window end is invalid, if inside of the rows that
 		 are invalidated below.  */
-	      if (INTEGERP (w->window_end_vpos)
-		  && XFASTINT (w->window_end_vpos) >= i)
+	      if (w->window_end_vpos >= i)
 		w->window_end_valid = Qnil;
 
 	      while (i < matrix->nrows)

=== modified file 'src/frame.c'
--- src/frame.c	2012-06-26 14:41:01 +0000
+++ src/frame.c	2012-06-27 04:52:47 +0000
@@ -131,7 +131,7 @@
 {
   struct window *w = XWINDOW (window);
 
-  XSETFASTINT (w->last_modified, 0);
+  w->last_modified = 0;
   XSETFASTINT (w->top_line, XFASTINT (w->top_line) + n);
   XSETFASTINT (w->total_lines, XFASTINT (w->total_lines) - n);
 
@@ -356,8 +356,7 @@
   f->selected_window = root_window;
   /* Make sure this window seems more recently used than
      a newly-created, never-selected window.  */
-  ++window_select_count;
-  XSETFASTINT (XWINDOW (f->selected_window)->use_time, window_select_count);
+  XWINDOW (f->selected_window)->use_time = ++window_select_count;
 
   return f;
 }

=== modified file 'src/minibuf.c'
--- src/minibuf.c	2012-06-19 17:05:41 +0000
+++ src/minibuf.c	2012-06-27 04:52:47 +0000
@@ -888,8 +888,8 @@
 
   /* Make sure minibuffer window is erased, not ignored.  */
   windows_or_buffers_changed++;
-  XSETFASTINT (XWINDOW (window)->last_modified, 0);
-  XSETFASTINT (XWINDOW (window)->last_overlay_modified, 0);
+  XWINDOW (window)->last_modified = 0;
+  XWINDOW (window)->last_overlay_modified = 0;
 
   /* In case the previous minibuffer displayed in this miniwindow is
      dead, we may keep displaying this buffer (tho it's inactive), so reset it,

=== modified file 'src/print.c'
--- src/print.c	2012-06-26 02:33:51 +0000
+++ src/print.c	2012-06-27 04:52:47 +0000
@@ -1774,8 +1774,7 @@
 	{
 	  int len;
 	  strout ("#<window ", -1, -1, printcharfun);
-	  len = sprintf (buf, "%"pI"d",
-			 XFASTINT (XWINDOW (obj)->sequence_number));
+	  len = sprintf (buf, "%d", XWINDOW (obj)->sequence_number);
 	  strout (buf, len, len, printcharfun);
 	  if (!NILP (XWINDOW (obj)->buffer))
 	    {

=== modified file 'src/window.c'
--- src/window.c	2012-06-26 14:41:01 +0000
+++ src/window.c	2012-06-27 04:52:47 +0000
@@ -117,9 +117,6 @@
 /* Incremented for each window created.  */
 static int sequence_number;
 
-/* Nonzero after init_window_once has finished.  */
-static int window_initialized;
-
 /* Hook to run when window config changes.  */
 static Lisp_Object Qwindow_configuration_change_hook;
 
@@ -331,8 +328,7 @@
 
   if (NILP (norecord))
     {
-      ++window_select_count;
-      XSETFASTINT (w->use_time, window_select_count);
+      w->use_time = ++window_select_count;
       record_buffer (w->buffer);
     }
 
@@ -499,7 +495,7 @@
 selected one.  */)
   (Lisp_Object window)
 {
-  return decode_window (window)->use_time;
+  return make_number (decode_window (window)->use_time);
 }
 \f
 DEFUN ("window-total-height", Fwindow_total_height, Swindow_total_height, 0, 1, 0,
@@ -1314,8 +1310,8 @@
 
   if (! NILP (update)
       && ! (! NILP (w->window_end_valid)
-	    && XFASTINT (w->last_modified) >= BUF_MODIFF (b)
-	    && XFASTINT (w->last_overlay_modified) >= BUF_OVERLAY_MODIFF (b))
+	    && w->last_modified >= BUF_MODIFF (b)
+	    && w->last_overlay_modified >= BUF_OVERLAY_MODIFF (b))
       && !noninteractive)
     {
       struct text_pos startp;
@@ -1355,7 +1351,7 @@
 	set_buffer_internal (old_buffer);
     }
   else
-    XSETINT (value, BUF_Z (b) - XFASTINT (w->window_end_pos));
+    XSETINT (value, BUF_Z (b) - w->window_end_pos);
 
   return value;
 }
@@ -1398,8 +1394,8 @@
   if (NILP (noforce))
     w->force_start = 1;
   w->update_mode_line = 1;
-  XSETFASTINT (w->last_modified, 0);
-  XSETFASTINT (w->last_overlay_modified, 0);
+  w->last_modified = 0;
+  w->last_overlay_modified = 0;
   if (!EQ (window, selected_window))
     windows_or_buffers_changed++;
 
@@ -1511,8 +1507,8 @@
   if (NILP (w->window_end_valid)
       || current_buffer->clip_changed
       || current_buffer->prevent_redisplay_optimizations_p
-      || XFASTINT (w->last_modified) < BUF_MODIFF (b)
-      || XFASTINT (w->last_overlay_modified) < BUF_OVERLAY_MODIFF (b))
+      || w->last_modified < BUF_MODIFF (b)
+      || w->last_overlay_modified < BUF_OVERLAY_MODIFF (b))
     return Qnil;
 
   if (NILP (line))
@@ -1842,8 +1838,8 @@
       n->phys_cursor_width = -1;
       n->must_be_updated_p = 0;
       n->pseudo_window_p = 0;
-      XSETFASTINT (n->window_end_vpos, 0);
-      XSETFASTINT (n->window_end_pos, 0);
+      n->window_end_vpos = 0;
+      n->window_end_pos = 0;
       n->window_end_valid = Qnil;
       n->frozen_window_start_p = 0;
     }
@@ -2990,8 +2986,8 @@
     XSETINT (BVAR (b, display_count), XINT (BVAR (b, display_count)) + 1);
   BVAR (b, display_time) = Fcurrent_time ();
 
-  XSETFASTINT (w->window_end_pos, 0);
-  XSETFASTINT (w->window_end_vpos, 0);
+  w->window_end_pos = 0;
+  w->window_end_vpos = 0;
   memset (&w->last_cursor, 0, sizeof w->last_cursor);
   w->window_end_valid = Qnil;
   if (!(keep_margins_p && samebuf))
@@ -3011,8 +3007,8 @@
 			     buffer);
       w->start_at_line_beg = 0;
       w->force_start = 0;
-      XSETFASTINT (w->last_modified, 0);
-      XSETFASTINT (w->last_overlay_modified, 0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
     }
   /* Maybe we could move this into the `if' but it's not obviously safe and
      I doubt it's worth the trouble.  */
@@ -3021,7 +3017,7 @@
   /* We must select BUFFER for running the window-scroll-functions.  */
   /* We can't check ! NILP (Vwindow_scroll_functions) here
      because that might itself be a local variable.  */
-  if (window_initialized)
+  if (initialized)
     {
       record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
       Fset_buffer (buffer);
@@ -3250,8 +3246,7 @@
 	  sizeof (Lisp_Object) * VECSIZE (struct window));
   XSETWINDOW (parent, p);
 
-  ++sequence_number;
-  XSETFASTINT (p->sequence_number, sequence_number);
+  p->sequence_number = ++sequence_number;
 
   replace_window (window, parent, 1);
 
@@ -3290,17 +3285,12 @@
   w->pointm = Fmake_marker ();
   XSETFASTINT (w->hscroll, 0);
   XSETFASTINT (w->min_hscroll, 0);
-  XSETFASTINT (w->use_time, 0);
-  ++sequence_number;
-  XSETFASTINT (w->sequence_number, sequence_number);
-  XSETFASTINT (w->last_point, 0);
   w->vertical_scroll_bar_type = Qt;
-  XSETFASTINT (w->window_end_pos, 0);
-  XSETFASTINT (w->window_end_vpos, 0);
 
   /* Initialize non-Lisp data.  Note that allocate_window zeroes out all
      non-Lisp data, so do it only for slots which should not be zero.  */
   w->nrows_scale_factor = w->ncols_scale_factor = 1;
+  w->sequence_number = ++sequence_number;
   w->phys_cursor_type = -1;
   w->phys_cursor_width = -1;
 
@@ -3492,8 +3482,8 @@
     }
 
   /* Clear out some redisplay caches.  */
-  XSETFASTINT (w->last_modified, 0);
-  XSETFASTINT (w->last_overlay_modified, 0);
+  w->last_modified = 0;
+  w->last_overlay_modified = 0;
 }
 
 
@@ -4025,8 +4015,8 @@
       /* Grow the mini-window.  */
       XSETFASTINT (w->top_line, XFASTINT (r->top_line) + XFASTINT (r->total_lines));
       XSETFASTINT (w->total_lines, XFASTINT (w->total_lines) - XINT (value));
-      XSETFASTINT (w->last_modified, 0);
-      XSETFASTINT (w->last_overlay_modified, 0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
 
       adjust_glyphs (f);
       UNBLOCK_INPUT;
@@ -4061,8 +4051,8 @@
 	  XSETFASTINT (w->top_line, XFASTINT (r->top_line) + XFASTINT (r->total_lines));
 	  XSETFASTINT (w->total_lines, 1);
 
-	  XSETFASTINT (w->last_modified, 0);
-	  XSETFASTINT (w->last_overlay_modified, 0);
+	  w->last_modified = 0;
+	  w->last_overlay_modified = 0;
 
 	  adjust_glyphs (f);
 	  UNBLOCK_INPUT;
@@ -4289,8 +4279,8 @@
 					 w->buffer);
 		  w->start_at_line_beg = 1;
 		  w->update_mode_line = 1;
-		  XSETFASTINT (w->last_modified, 0);
-		  XSETFASTINT (w->last_overlay_modified, 0);
+		  w->last_modified = 0;
+		  w->last_overlay_modified = 0;
 		  /* Set force_start so that redisplay_window will run the
 		     window-scroll-functions.  */
 		  w->force_start = 1;
@@ -4435,8 +4425,8 @@
       bytepos = XMARKER (w->start)->bytepos;
       w->start_at_line_beg = (pos == BEGV || FETCH_BYTE (bytepos - 1) == '\n');
       w->update_mode_line = 1;
-      XSETFASTINT (w->last_modified, 0);
-      XSETFASTINT (w->last_overlay_modified, 0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
       /* Set force_start so that redisplay_window will run the
 	 window-scroll-functions.  */
       w->force_start = 1;
@@ -4634,8 +4624,8 @@
       set_marker_restricted_both (w->start, w->buffer, pos, pos_byte);
       w->start_at_line_beg = !NILP (bolp);
       w->update_mode_line = 1;
-      XSETFASTINT (w->last_modified, 0);
-      XSETFASTINT (w->last_overlay_modified, 0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
       /* Set force_start so that redisplay_window will run
 	 the window-scroll-functions.  */
       w->force_start = 1;
@@ -5575,8 +5565,8 @@
 		}
 	    }
 
-	  XSETFASTINT (w->last_modified, 0);
-	  XSETFASTINT (w->last_overlay_modified, 0);
+	  w->last_modified = 0;
+	  w->last_overlay_modified = 0;
 
 	  /* Reinstall the saved buffer and pointers into it.  */
 	  if (NILP (p->buffer))
@@ -6478,8 +6468,6 @@
   minibuf_window = f->minibuffer_window;
   selected_window = f->selected_window;
   last_nonminibuf_frame = f;
-
-  window_initialized = 1;
 }
 
 void

=== modified file 'src/window.h'
--- src/window.h	2012-06-01 03:41:03 +0000
+++ src/window.h	2012-06-27 06:24:25 +0000
@@ -147,24 +147,10 @@
        the user has set, by set-window-hscroll for example.  */
     Lisp_Object min_hscroll;
 
-    /* Number saying how recently window was selected.  */
-    Lisp_Object use_time;
-
-    /* Unique number of window assigned when it was created.  */
-    Lisp_Object sequence_number;
-
     /* No permanent meaning; used by save-window-excursion's
        bookkeeping.  */
     Lisp_Object temslot;
 
-    /* text.modified of displayed buffer as of last time display
-       completed.  */
-    Lisp_Object last_modified;
-    /* BUF_OVERLAY_MODIFIED of displayed buffer as of last complete update.  */
-    Lisp_Object last_overlay_modified;
-    /* Value of point at that time.  */
-    Lisp_Object last_point;
-
     /* This window's vertical scroll bar.  This field is only for use
        by the window-system-dependent code which implements the
        scroll bars; it can store anything it likes here.  If this
@@ -191,16 +177,10 @@
        no scroll bar.  A value of t means use frame value.  */
     Lisp_Object vertical_scroll_bar_type;
 
-    /* Z - the buffer position of the last glyph in the current matrix
-       of W.  Only valid if WINDOW_END_VALID is not nil.  */
-    Lisp_Object window_end_pos;
-    /* Glyph matrix row of the last glyph in the current matrix
-       of W.  Only valid if WINDOW_END_VALID is not nil.  */
-    Lisp_Object window_end_vpos;
-    /* t if window_end_pos is truly valid.
-       This is nil if nontrivial redisplay is preempted
-       since in that case the frame image that window_end_pos
-       did not get onto the frame.  */
+    /* t if window_end_pos is truly valid.  This is nil if nontrivial
+       redisplay is preempted since in that case the frame image that
+       window_end_pos did not get onto the frame.  FIXME: check whether
+       this may be converted to a bitfield or even eliminated at all.  */
     Lisp_Object window_end_valid;
 
     /* Display-table to use for displaying chars in this window.
@@ -254,6 +234,31 @@
     struct glyph_matrix *current_matrix;
     struct glyph_matrix *desired_matrix;
 
+    /* Number saying how recently window was selected.  */
+    int use_time;
+
+    /* Unique number of window assigned when it was created.  */
+    int sequence_number;
+
+    /* 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;
+
+    /* Z - the buffer position of the last glyph in the current matrix
+       of W.  Only valid if WINDOW_END_VALID is not nil.  */
+    ptrdiff_t window_end_pos;
+
+    /* Glyph matrix row of the last glyph in the current matrix
+       of W.  Only valid if WINDOW_END_VALID is not nil.  */
+    ptrdiff_t window_end_vpos;
+
     /* Scaling factor for the glyph_matrix size calculation in this window.
        Used if window contains many small images or uses proportional fonts,
        as the normal  may yield a matrix which is too small.  */

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2012-06-26 02:33:51 +0000
+++ src/xdisp.c	2012-06-27 06:57:43 +0000
@@ -2508,7 +2508,7 @@
     {
       struct glyph_row *row;
       xassert ((row = MATRIX_ROW (w->current_matrix,
-				  XFASTINT (w->window_end_vpos)),
+				  w->window_end_vpos),
 		!row->enabled_p
 		|| MATRIX_ROW_DISPLAYS_TEXT_P (row)
 		|| MATRIX_ROW_VPOS (row, w->current_matrix) == 0));
@@ -8850,7 +8850,7 @@
       && it->current_x == it->last_visible_x - 1
       && it->c != '\n'
       && it->c != '\t'
-      && it->vpos < XFASTINT (it->w->window_end_vpos))
+      && it->vpos < it->w->window_end_vpos)
     {
       it->continuation_lines_width += it->current_x;
       it->current_x = it->hpos = it->max_ascent = it->max_descent = 0;
@@ -12508,8 +12508,8 @@
   int unchanged_p = 1;
 
   /* If text or overlays have changed, see where.  */
-  if (XFASTINT (w->last_modified) < MODIFF
-      || XFASTINT (w->last_overlay_modified) < OVERLAY_MODIFF)
+  if (w->last_modified < MODIFF
+      || w->last_overlay_modified < OVERLAY_MODIFF)
     {
       /* Gap in the line?  */
       if (GPT < start || Z - GPT < end)
@@ -12788,9 +12788,9 @@
 	pt = marker_position (w->pointm);
 
       if ((w->current_matrix->buffer != XBUFFER (w->buffer)
-	   || pt != XINT (w->last_point))
+	   || pt != w->last_point)
 	  && check_point_in_composition (w->current_matrix->buffer,
-					 XINT (w->last_point),
+					 w->last_point,
 					 XBUFFER (w->buffer), pt))
 	b->clip_changed = 1;
     }
@@ -13012,9 +13012,9 @@
   if (!NILP (w->column_number_displayed)
       /* This alternative quickly identifies a common case
 	 where no change is needed.  */
-      && !(PT == XFASTINT (w->last_point)
-	   && XFASTINT (w->last_modified) >= MODIFF
-	   && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF)
+      && !(PT == w->last_point
+	   && w->last_modified >= MODIFF
+	   && w->last_overlay_modified >= OVERLAY_MODIFF)
       && (XFASTINT (w->column_number_displayed) != current_column ()))
     w->update_mode_line = 1;
 
@@ -13077,8 +13077,8 @@
     }
   else if (EQ (selected_window, minibuf_window)
 	   && (current_buffer->clip_changed
-	       || XFASTINT (w->last_modified) < MODIFF
-	       || XFASTINT (w->last_overlay_modified) < OVERLAY_MODIFF)
+	       || w->last_modified < MODIFF
+	       || w->last_overlay_modified < OVERLAY_MODIFF)
 	   && resize_mini_window (w, 0))
     {
       /* Resized active mini-window to fit the size of what it is
@@ -13143,8 +13143,8 @@
 	      || FETCH_BYTE (BYTEPOS (tlbufpos)) == '\n'))
 	/* Former continuation line has disappeared by becoming empty.  */
 	goto cancel;
-      else if (XFASTINT (w->last_modified) < MODIFF
-	       || XFASTINT (w->last_overlay_modified) < OVERLAY_MODIFF
+      else if (w->last_modified < MODIFF
+	       || w->last_overlay_modified < OVERLAY_MODIFF
 	       || MINI_WINDOW_P (w))
 	{
 	  /* We have to handle the case of continuation around a
@@ -13224,12 +13224,12 @@
 		 adjusted.  */
 	      if ((it.glyph_row - 1)->displays_text_p)
 		{
-		  if (XFASTINT (w->window_end_vpos) < this_line_vpos)
-		    XSETINT (w->window_end_vpos, this_line_vpos);
+		  if (w->window_end_vpos < this_line_vpos)
+		    w->window_end_vpos = this_line_vpos;
 		}
-	      else if (XFASTINT (w->window_end_vpos) == this_line_vpos
+	      else if (w->window_end_vpos == this_line_vpos
 		       && this_line_vpos > 0)
-		XSETINT (w->window_end_vpos, this_line_vpos - 1);
+		w->window_end_vpos = this_line_vpos - 1;
 	      w->window_end_valid = Qnil;
 
 	      /* Update hint: No need to try to scroll in update_window.  */
@@ -13248,7 +13248,7 @@
 	    goto cancel;
 	}
       else if (/* Cursor position hasn't changed.  */
-	       PT == XFASTINT (w->last_point)
+	       PT == w->last_point
 	       /* Make sure the cursor was last displayed
 		  in this window.  Otherwise we have to reposition it.  */
 	       && 0 <= w->cursor.vpos
@@ -13664,10 +13664,8 @@
     {
       struct buffer *b = XBUFFER (w->buffer);
 
-      w->last_modified
-	= make_number (accurate_p ? BUF_MODIFF (b) : 0);
-      w->last_overlay_modified
-	= make_number (accurate_p ? BUF_OVERLAY_MODIFF (b) : 0);
+      w->last_modified = accurate_p ? BUF_MODIFF (b) : 0;
+      w->last_overlay_modified = accurate_p ? BUF_OVERLAY_MODIFF (b) : 0;
       w->last_had_star
 	= BUF_MODIFF (b) > BUF_SAVE_MODIFF (b);
 
@@ -13689,9 +13687,9 @@
 	  w->last_cursor_off_p = w->cursor_off_p;
 
 	  if (w == XWINDOW (selected_window))
-	    w->last_point = make_number (BUF_PT (b));
+	    w->last_point = BUF_PT (b);
 	  else
-	    w->last_point = make_number (XMARKER (w->pointm)->charpos);
+	    w->last_point = XMARKER (w->pointm)->charpos;
 	}
     }
 
@@ -14932,6 +14930,11 @@
     return rc;
 #endif
 
+  /* Previously there was a runtime check whether last_point is non-nil.
+     Now it's converted to ptrdiff_t, so zero means invalid position in
+     a buffer.  */
+  eassert (w->last_point > 0);
+
   /* Handle case where text has not changed, only point, and it has
      not moved off the frame.  */
   if (/* Point may be in this window.  */
@@ -14952,8 +14955,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)
       /* This code is not used for mini-buffer for the sake of the case
 	 of redisplaying to replace an echo area message; since in
 	 that case the mini-buffer contents per se are usually
@@ -14962,12 +14963,11 @@
 	 handles the same cases.  */
       && !EQ (window, minibuf_window)
       /* When splitting windows or for new windows, it happens that
-	 redisplay is called with a nil window_end_vpos or one being
-	 larger than the window.  This should really be fixed in
-	 window.c.  I don't have this on my list, now, so we do
-	 approximately the same as the old redisplay code.  --gerd.  */
-      && INTEGERP (w->window_end_vpos)
-      && XFASTINT (w->window_end_vpos) < w->current_matrix->nrows
+	 redisplay is called with a zero window_end_vpos or one being
+	 larger than the window.  FIXME: Gerd has said that this should
+	 be fixed in window.c; anyway, this needs to be checked.  */
+      && w->window_end_vpos > 0
+      && w->window_end_vpos < w->current_matrix->nrows
       && (FRAME_WINDOW_P (f)
 	  || !overlay_arrow_in_current_buffer_p ()))
     {
@@ -15011,7 +15011,7 @@
 	  int scroll_p = 0, must_scroll = 0;
 	  int last_y = window_text_bottom_y (w) - this_scroll_margin;
 
-	  if (PT > XFASTINT (w->last_point))
+	  if (PT > w->last_point)
 	    {
 	      /* Point has moved forward.  */
 	      while (MATRIX_ROW_END_CHARPOS (row) < PT
@@ -15046,7 +15046,7 @@
 		      && !MATRIX_ROW_ENDS_IN_MIDDLE_OF_CHAR_P (row)))
 		scroll_p = 1;
 	    }
-	  else if (PT < XFASTINT (w->last_point))
+	  else if (PT < w->last_point)
 	    {
 	      /* Cursor has to be moved backward.  Note that PT >=
 		 CHARPOS (startp) because of the outer if-statement.  */
@@ -15279,7 +15279,7 @@
       start = marker_position (w->start) - BUF_BEGV (buf);
       /* I don't think this is guaranteed to be right.  For the
 	 moment, we'll pretend it is.  */
-      end = BUF_Z (buf) - XFASTINT (w->window_end_pos) - BUF_BEGV (buf);
+      end = BUF_Z (buf) - w->window_end_pos - BUF_BEGV (buf);
 
       if (end < start)
 	end = start;
@@ -15392,8 +15392,8 @@
     = (!NILP (w->window_end_valid)
        && !current_buffer->clip_changed
        && !current_buffer->prevent_redisplay_optimizations_p
-       && XFASTINT (w->last_modified) >= MODIFF
-       && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF);
+       && w->last_modified >= MODIFF
+       && w->last_overlay_modified >= OVERLAY_MODIFF);
 
   /* Run the window-bottom-change-functions
      if it is possible that the text on the screen has changed
@@ -15415,8 +15415,8 @@
   buffer_unchanged_p
     = (!NILP (w->window_end_valid)
        && !current_buffer->clip_changed
-       && XFASTINT (w->last_modified) >= MODIFF
-       && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF);
+       && w->last_modified >= MODIFF
+       && w->last_overlay_modified >= OVERLAY_MODIFF);
 
   /* When windows_or_buffers_changed is non-zero, we can't rely on
      the window end being valid, so set it to nil there.  */
@@ -15441,9 +15441,9 @@
   if (!NILP (w->column_number_displayed)
       /* This alternative quickly identifies a common case
 	 where no change is needed.  */
-      && !(PT == XFASTINT (w->last_point)
-	   && XFASTINT (w->last_modified) >= MODIFF
-	   && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF)
+      && !(PT == w->last_point
+	   && w->last_modified >= MODIFF
+	   && w->last_overlay_modified >= OVERLAY_MODIFF)
       && (XFASTINT (w->column_number_displayed) != current_column ()))
     update_mode_line = 1;
 
@@ -15559,8 +15559,8 @@
 	  startp = run_window_scroll_functions (window, startp);
 	}
 
-      w->last_modified = make_number (0);
-      w->last_overlay_modified = make_number (0);
+      w->last_modified = 0;
+      w->last_overlay_modified = 0;
       if (CHARPOS (startp) < BEGV)
 	SET_TEXT_POS (startp, BEGV, BEGV_BYTE);
       else if (CHARPOS (startp) > ZV)
@@ -15685,8 +15685,8 @@
 	   && (CHARPOS (startp) < ZV
 	       /* Avoid starting at end of buffer.  */
 	       || CHARPOS (startp) == BEGV
-	       || (XFASTINT (w->last_modified) >= MODIFF
-		   && XFASTINT (w->last_overlay_modified) >= OVERLAY_MODIFF)))
+	       || (w->last_modified >= MODIFF
+		   && w->last_overlay_modified >= OVERLAY_MODIFF)))
     {
       int d1, d2, d3, d4, d5, d6;
 
@@ -15773,8 +15773,8 @@
 
  try_to_scroll:
 
-  w->last_modified = make_number (0);
-  w->last_overlay_modified = make_number (0);
+  w->last_modified = 0;
+  w->last_overlay_modified = 0;
 
   /* Redisplay the mode line.  Select the buffer properly for that.  */
   if (!update_mode_line)
@@ -15962,7 +15962,7 @@
   if (w->cursor.vpos < 0)
     {
       if (!NILP (w->window_end_valid)
-	  && PT >= Z - XFASTINT (w->window_end_pos))
+	  && PT >= Z - w->window_end_pos)
 	{
 	  clear_glyph_matrix (w->desired_matrix);
 	  move_it_by_lines (&it, 1);
@@ -16251,7 +16251,7 @@
     }
 
   /* If bottom moved off end of frame, change mode line percentage.  */
-  if (XFASTINT (w->window_end_pos) <= 0
+  if (w->window_end_pos <= 0
       && Z != IT_CHARPOS (it))
     w->update_mode_line = 1;
 
@@ -16263,18 +16263,16 @@
       xassert (MATRIX_ROW_DISPLAYS_TEXT_P (last_text_row));
       w->window_end_bytepos
 	= Z_BYTE - MATRIX_ROW_END_BYTEPOS (last_text_row);
-      w->window_end_pos
-	= make_number (Z - MATRIX_ROW_END_CHARPOS (last_text_row));
-      w->window_end_vpos
-	= make_number (MATRIX_ROW_VPOS (last_text_row, w->desired_matrix));
-      xassert (MATRIX_ROW (w->desired_matrix, XFASTINT (w->window_end_vpos))
+      w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (last_text_row);
+      w->window_end_vpos = MATRIX_ROW_VPOS (last_text_row, w->desired_matrix);
+      xassert (MATRIX_ROW (w->desired_matrix, w->window_end_vpos)
 	       ->displays_text_p);
     }
   else
     {
       w->window_end_bytepos = Z_BYTE - ZV_BYTE;
-      w->window_end_pos = make_number (Z - ZV);
-      w->window_end_vpos = make_number (0);
+      w->window_end_pos = Z - ZV;
+      w->window_end_vpos = 0;
     }
 
   /* But that is not valid info until redisplay finishes.  */
@@ -16502,27 +16500,22 @@
 	{
 	  w->window_end_bytepos
 	    = Z_BYTE - MATRIX_ROW_END_BYTEPOS (last_reused_text_row);
-	  w->window_end_pos
-	    = make_number (Z - MATRIX_ROW_END_CHARPOS (last_reused_text_row));
-	  w->window_end_vpos
-	    = make_number (MATRIX_ROW_VPOS (last_reused_text_row,
-					    w->current_matrix));
+	  w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (last_reused_text_row);
+	  w->window_end_vpos = MATRIX_ROW_VPOS (last_reused_text_row, w->current_matrix);
 	}
       else if (last_text_row)
 	{
 	  w->window_end_bytepos
 	    = Z_BYTE - MATRIX_ROW_END_BYTEPOS (last_text_row);
-	  w->window_end_pos
-	    = make_number (Z - MATRIX_ROW_END_CHARPOS (last_text_row));
-	  w->window_end_vpos
-	    = make_number (MATRIX_ROW_VPOS (last_text_row, w->desired_matrix));
+	  w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (last_text_row);
+	  w->window_end_vpos = MATRIX_ROW_VPOS (last_text_row, w->desired_matrix);
 	}
       else
 	{
 	  /* This window must be completely empty.  */
 	  w->window_end_bytepos = Z_BYTE - ZV_BYTE;
-	  w->window_end_pos = make_number (Z - ZV);
-	  w->window_end_vpos = make_number (0);
+	  w->window_end_pos = Z - ZV;
+	  w->window_end_vpos = 0;
 	}
       w->window_end_valid = Qnil;
 
@@ -16705,15 +16698,13 @@
 	{
 	  w->window_end_bytepos
 	    = Z_BYTE - MATRIX_ROW_END_BYTEPOS (last_text_row);
-	  w->window_end_pos
-	    = make_number (Z - MATRIX_ROW_END_CHARPOS (last_text_row));
-	  w->window_end_vpos
-	    = make_number (MATRIX_ROW_VPOS (last_text_row, w->desired_matrix));
+	  w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (last_text_row);
+	  w->window_end_vpos = MATRIX_ROW_VPOS (last_text_row, w->desired_matrix);
 	}
       else
 	{
 	  w->window_end_vpos
-	    = make_number (XFASTINT (w->window_end_vpos) - nrows_scrolled);
+	    = make_number (w->window_end_vpos - nrows_scrolled);
 	}
 
       w->window_end_valid = Qnil;
@@ -16854,11 +16845,11 @@
   /* A value of window_end_pos >= END_UNCHANGED means that the window
      end is in the range of changed text.  If so, there is no
      unchanged row at the end of W's current matrix.  */
-  if (XFASTINT (w->window_end_pos) >= END_UNCHANGED)
+  if (w->window_end_pos >= END_UNCHANGED)
     return NULL;
 
   /* Set row to the last row in W's current matrix displaying text.  */
-  row = MATRIX_ROW (w->current_matrix, XFASTINT (w->window_end_vpos));
+  row = MATRIX_ROW (w->current_matrix, w->window_end_vpos);
 
   /* If matrix is entirely empty, no unchanged row exists.  */
   if (MATRIX_ROW_DISPLAYS_TEXT_P (row))
@@ -16869,7 +16860,7 @@
 	 buffer positions in the current matrix to current buffer
 	 positions for characters not in changed text.  */
       ptrdiff_t Z_old =
-	MATRIX_ROW_END_CHARPOS (row) + XFASTINT (w->window_end_pos);
+	MATRIX_ROW_END_CHARPOS (row) + w->window_end_pos;
       ptrdiff_t Z_BYTE_old =
 	MATRIX_ROW_END_BYTEPOS (row) + w->window_end_bytepos;
       ptrdiff_t last_unchanged_pos, last_unchanged_pos_old;
@@ -17133,7 +17124,7 @@
     GIVE_UP (5);
 
   /* Another way to prevent redisplay optimizations.  */
-  if (XFASTINT (w->last_modified) == 0)
+  if (w->last_modified == 0)
     GIVE_UP (6);
 
   /* Verify that window is not hscrolled.  */
@@ -17205,7 +17196,7 @@
      This case happens with stealth-fontification.  Note that although
      the display is unchanged, glyph positions in the matrix have to
      be adjusted, of course.  */
-  row = MATRIX_ROW (w->current_matrix, XFASTINT (w->window_end_vpos));
+  row = MATRIX_ROW (w->current_matrix, w->window_end_vpos);
   if (MATRIX_ROW_DISPLAYS_TEXT_P (row)
       && ((last_changed_charpos < CHARPOS (start)
 	   && CHARPOS (start) == BEGV)
@@ -17217,7 +17208,7 @@
 
       /* Compute how many chars/bytes have been added to or removed
 	 from the buffer.  */
-      Z_old = MATRIX_ROW_END_CHARPOS (row) + XFASTINT (w->window_end_pos);
+      Z_old = MATRIX_ROW_END_CHARPOS (row) + w->window_end_pos;
       Z_BYTE_old = MATRIX_ROW_END_BYTEPOS (row) + w->window_end_bytepos;
       Z_delta = Z - Z_old;
       Z_delta_bytes = Z_BYTE - Z_BYTE_old;
@@ -17288,8 +17279,7 @@
 	{
 	  /* We have to compute the window end anew since text
 	     could have been added/removed after it.  */
-	  w->window_end_pos
-	    = make_number (Z - MATRIX_ROW_END_CHARPOS (row));
+	  w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (row);
 	  w->window_end_bytepos
 	    = Z_BYTE - MATRIX_ROW_END_BYTEPOS (row);
 
@@ -17324,7 +17314,7 @@
 
   /* Give up if the window ends in strings.  Overlay strings
      at the end are difficult to handle, so don't try.  */
-  row = MATRIX_ROW (current_matrix, XFASTINT (w->window_end_vpos));
+  row = MATRIX_ROW (current_matrix, w->window_end_vpos);
   if (MATRIX_ROW_START_CHARPOS (row) == MATRIX_ROW_END_CHARPOS (row))
     GIVE_UP (20);
 
@@ -17667,7 +17657,7 @@
       /* Set last_row to the glyph row in the current matrix where the
 	 window end line is found.  It has been moved up or down in
 	 the matrix by dvpos.  */
-      int last_vpos = XFASTINT (w->window_end_vpos) + dvpos;
+      int last_vpos = w->window_end_vpos + dvpos;
       struct glyph_row *last_row = MATRIX_ROW (current_matrix, last_vpos);
 
       /* If last_row is the window end line, it should display text.  */
@@ -17723,21 +17713,18 @@
 					   first_unchanged_at_end_row);
       xassert (row && MATRIX_ROW_DISPLAYS_TEXT_P (row));
 
-      w->window_end_pos = make_number (Z - MATRIX_ROW_END_CHARPOS (row));
+      w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (row);
       w->window_end_bytepos = Z_BYTE - MATRIX_ROW_END_BYTEPOS (row);
-      w->window_end_vpos
-	= make_number (MATRIX_ROW_VPOS (row, w->current_matrix));
+      w->window_end_vpos = MATRIX_ROW_VPOS (row, w->current_matrix);
       xassert (w->window_end_bytepos >= 0);
       IF_DEBUG (debug_method_add (w, "A"));
     }
   else if (last_text_row_at_end)
     {
-      w->window_end_pos
-	= make_number (Z - MATRIX_ROW_END_CHARPOS (last_text_row_at_end));
+      w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (last_text_row_at_end);
       w->window_end_bytepos
 	= Z_BYTE - MATRIX_ROW_END_BYTEPOS (last_text_row_at_end);
-      w->window_end_vpos
-	= make_number (MATRIX_ROW_VPOS (last_text_row_at_end, desired_matrix));
+      w->window_end_vpos = MATRIX_ROW_VPOS (last_text_row_at_end, desired_matrix);
       xassert (w->window_end_bytepos >= 0);
       IF_DEBUG (debug_method_add (w, "B"));
     }
@@ -17746,12 +17733,10 @@
       /* We have displayed either to the end of the window or at the
 	 end of the window, i.e. the last row with text is to be found
 	 in the desired matrix.  */
-      w->window_end_pos
-	= make_number (Z - MATRIX_ROW_END_CHARPOS (last_text_row));
+      w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (last_text_row);
       w->window_end_bytepos
 	= Z_BYTE - MATRIX_ROW_END_BYTEPOS (last_text_row);
-      w->window_end_vpos
-	= make_number (MATRIX_ROW_VPOS (last_text_row, desired_matrix));
+      w->window_end_vpos = MATRIX_ROW_VPOS (last_text_row, desired_matrix);
       xassert (w->window_end_bytepos >= 0);
     }
   else if (first_unchanged_at_end_row == NULL
@@ -17761,7 +17746,7 @@
       /* Displayed to end of window, but no line containing text was
 	 displayed.  Lines were deleted at the end of the window.  */
       int first_vpos = WINDOW_WANTS_HEADER_LINE_P (w) ? 1 : 0;
-      int vpos = XFASTINT (w->window_end_vpos);
+      int vpos = w->window_end_vpos;
       struct glyph_row *current_row = current_matrix->rows + vpos;
       struct glyph_row *desired_row = desired_matrix->rows + vpos;
 
@@ -17779,8 +17764,8 @@
 	}
 
       xassert (row != NULL);
-      w->window_end_vpos = make_number (vpos + 1);
-      w->window_end_pos = make_number (Z - MATRIX_ROW_END_CHARPOS (row));
+      w->window_end_vpos = vpos + 1;
+      w->window_end_pos = Z - MATRIX_ROW_END_CHARPOS (row);
       w->window_end_bytepos = Z_BYTE - MATRIX_ROW_END_BYTEPOS (row);
       xassert (w->window_end_bytepos >= 0);
       IF_DEBUG (debug_method_add (w, "C"));
@@ -17788,8 +17773,8 @@
   else
     abort ();
 
-  IF_DEBUG (debug_end_pos = XFASTINT (w->window_end_pos);
-	    debug_end_vpos = XFASTINT (w->window_end_vpos));
+  IF_DEBUG (debug_end_pos = w->window_end_pos;
+	    debug_end_vpos = w->window_end_vpos);
 
   /* Record that display has not been completed.  */
   w->window_end_valid = Qnil;
@@ -21408,7 +21393,7 @@
 	ptrdiff_t pos = marker_position (w->start);
 	ptrdiff_t total = BUF_ZV (b) - BUF_BEGV (b);
 
-	if (XFASTINT (w->window_end_pos) <= BUF_Z (b) - BUF_ZV (b))
+	if (w->window_end_pos <= BUF_Z (b) - BUF_ZV (b))
 	  {
 	    if (pos <= BUF_BEGV (b))
 	      return "All";
@@ -21437,7 +21422,7 @@
     case 'P':
       {
 	ptrdiff_t toppos = marker_position (w->start);
-	ptrdiff_t botpos = BUF_Z (b) - XFASTINT (w->window_end_pos);
+	ptrdiff_t botpos = BUF_Z (b) - w->window_end_pos;
 	ptrdiff_t total = BUF_ZV (b) - BUF_BEGV (b);
 
 	if (botpos >= BUF_ZV (b))
@@ -26311,7 +26296,7 @@
   /* Find the rows corresponding to START_CHARPOS and END_CHARPOS.  */
   rows_from_pos_range (w, start_charpos, end_charpos, disp_string, &r1, &r2);
   if (r1 == NULL)
-    r1 = MATRIX_ROW (w->current_matrix, XFASTINT (w->window_end_vpos));
+    r1 = MATRIX_ROW (w->current_matrix, w->window_end_vpos);
   /* If the before-string or display-string contains newlines,
      rows_from_pos_range skips to its last row.  Move back.  */
   if (!NILP (before_string) || !NILP (disp_string))
@@ -26333,7 +26318,7 @@
     }
   if (r2 == NULL)
     {
-      r2 = MATRIX_ROW (w->current_matrix, XFASTINT (w->window_end_vpos));
+      r2 = MATRIX_ROW (w->current_matrix, w->window_end_vpos);
       hlinfo->mouse_face_past_end = 1;
     }
   else if (!NILP (after_string))
@@ -26341,7 +26326,7 @@
       /* If the after-string has newlines, advance to its last row.  */
       struct glyph_row *next;
       struct glyph_row *last
-	= MATRIX_ROW (w->current_matrix, XFASTINT (w->window_end_vpos));
+	= MATRIX_ROW (w->current_matrix, w->window_end_vpos);
 
       for (next = r2 + 1;
 	   next <= last
@@ -27410,8 +27395,8 @@
   b = XBUFFER (w->buffer);
   if (part == ON_TEXT
       && EQ (w->window_end_valid, w->buffer)
-      && XFASTINT (w->last_modified) == BUF_MODIFF (b)
-      && XFASTINT (w->last_overlay_modified) == BUF_OVERLAY_MODIFF (b))
+      && w->last_modified == BUF_MODIFF (b)
+      && w->last_overlay_modified == BUF_OVERLAY_MODIFF (b))
     {
       int hpos, vpos, dx, dy, area = LAST_AREA;
       ptrdiff_t pos;
@@ -27648,7 +27633,7 @@
 		  Lisp_Object lim2 =
 		    NILP (BVAR (XBUFFER (buffer), bidi_display_reordering))
 		    ? make_number (BUF_Z (XBUFFER (buffer))
-				   - XFASTINT (w->window_end_pos))
+				   - w->window_end_pos)
 		    : Qnil;
 
 		  if (NILP (overlay))

Re: 'struct window' cleanup #3

[Stefan Monnier]

> 2. Fields hscroll and min_hscroll are good candidates for conversion
>    too, but this requires some redesign of `struct saved_window', most
>    probably to yet another pseudovector.

Actually, you can just leave the struct saved_window unchanged, and
convert to/from Lisp_Object when copying the (min_)hscroll to/from the
struct saved_window.

>    /* Right after splitting windows, last_point may be nil.  */
>    && INTEGERP (w->last_point);
>    My tests never shows zero here, so I'm installing assertion instead.

Since positions are strictly positive, you can use -1 as meaning "nil".
(I recommend -1 over 0, because it's "more different").


        Stefan

Re: 'struct window' cleanup #2

[Eli Zaretskii]

> Date: Wed, 27 Jun 2012 09:06:37 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: dmantipov@yandex.ru, monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
>  > Why a "hack"?  Lisp object are good precisely for this reason: that
>  > you can give them values of different types of object.
> 
> window_end_valid is documented as
> 
>      /* t if window_end_pos is truly valid.
>         This is nil if nontrivial redisplay is preempted
>         since in that case the frame image that window_end_pos
>         did not get onto the frame.  */
>      Lisp_Object window_end_valid;
> 
> but the code in xdisp.c checks it usually just as
> 
>       !NILP (w->window_end_valid)
> 
> Consequently, the assignment
> 
>        w->window_end_valid = w->buffer;
> 
> looks fragile (one has to be sure that the NILP checks don't happen
> while it's set to the buffer since otherwise we end up trying again for
> no use) and at least contradicts the initial comment of try_window_id
> 
> /* Try to redisplay window W by reusing its existing display.  W's
>     current matrix must be up to date when this function is called,
>     i.e. window_end_valid must not be nil.
> 
> but apparently the current matrix is not up to date when the value is
> non-nil but some other buffer.  So unless this is better documented
> and/or the corresponding code cleaned up it remains a "hack" IMHO.

What you see happens a lot in Emacs sources: comments don't correspond
to what the code really does, and tell only part of the story.
Usually, someone changes the code, but fails to update the comments
(because these comments are in another file, where the struct is
defined, not where the change is being done).  I tripped on such
issues several times when working on display features (in conjunction
with the various flags and state variables in 'struct it').

That's why I asked Dmitry to update and improve the comments whenever
some variable or struct member is being updated/modified.

>  >>  Can you see whether the part
>  >>
>  >>        /* When splitting windows or for new windows, it happens that
>  >> 	 redisplay is called with a nil window_end_vpos or one being
>  >> 	 larger than the window.  This should really be fixed in
>  >> 	 window.c.  I don't have this on my list, now, so we do
>  >> 	 approximately the same as the old redisplay code.  --gerd.  */
>  >>        && INTEGERP (w->window_end_vpos)
>  >>
>  >> in xdisp.c still makes sense?
>  >
>  > No, there's no code anymore that sets it to nil.
> 
> Fine (apparently that was another "hack" in redisplay).

No, it's another instance of "change-code-but-don't-update-comments"
phenomenon.

Re: 'struct window' cleanup #3

[Eli Zaretskii]

> Date: Wed, 27 Jun 2012 11:10:34 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> 1. IIUC, `window_initialized' is redundant and obsolete.

Does it still work to run "./temacs -Q", after this change:

> -  if (window_initialized)
> +  if (initialized)

The reason I ask is because window_initialized was set non-zero in
init_window_once, which was called from emacs.c when 'initialized' is
zero.  So the above 'if' would trigger when 'initialized' was zero,
but after your change it will only trigger when 'initialized' is
non-zero.  Am I missing something?  'initialized' is zero when running
un-dumped Emacs, e.g. 'temacs'.


> 3. The check:
> 
>     && INTEGERP (w->window_end_vpos)
>     && XFASTINT (w->window_end_vpos) < w->current_matrix->nrows
> 
>     is transformed to:
> 
>     && w->window_end_vpos > 0
>     && w->window_end_vpos < w->current_matrix->nrows

I don't think this is right.  window_end_vpos is the number of the
last glyph row of a glyph matrix, so it can legitimately be zero, when
w->current_matrix->nrows is 1.  We need to initialize this member with
some invalid value, like -1, if we want to distinguish between values
we can and cannot trust.  Or maybe test window_end_valid here (to put
our money where our mouth -- i.e. the commentary -- is ;-).

>     I never get w->window_end_vpos >= w->current_matrix->nrows

Try something that enlarges and then shrinks the minibuffer window.
Or maybe split a window on a TTY.  Anyway, this does happen.

>     but run into w->window_end_vpos == 0 several times.

See above: it's a legitimate value.

>     At this moment, I have no ideas whether Gerd was correct about
>     fixing window.c, so this needs more detailed investigations.

My gray hair taught me that Gerd is usually right.

Re: 'struct window' cleanup #3

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 27 Jun 2012 09:32:15 -0400
> Cc: , emacs-devel@gnu.org
> 
> >    /* Right after splitting windows, last_point may be nil.  */
> >    && INTEGERP (w->last_point);
> >    My tests never shows zero here, so I'm installing assertion instead.
> 
> Since positions are strictly positive, you can use -1 as meaning "nil".
> (I recommend -1 over 0, because it's "more different").

I agree, but beware of those 'if (!w->last_point)' gotchas...

Re: 'struct window' cleanup #3

[Dmitry Antipov]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On 06/27/2012 09:24 PM, Eli Zaretskii wrote:
>> Date: Wed, 27 Jun 2012 11:10:34 +0400
>> From: Dmitry Antipov <dmantipov@yandex.ru>
>> CC: emacs-devel@gnu.org
>>
>> 1. IIUC, `window_initialized' is redundant and obsolete.
>
> Does it still work to run "./temacs -Q", after this change:
>
>> -  if (window_initialized)
>> +  if (initialized)
>
> The reason I ask is because window_initialized was set non-zero in
> init_window_once, which was called from emacs.c when 'initialized' is
> zero.  So the above 'if' would trigger when 'initialized' was zero,
> but after your change it will only trigger when 'initialized' is
> non-zero.  Am I missing something?  'initialized' is zero when running
> un-dumped Emacs, e.g. 'temacs'.

I would like to propose alternate redesign for this: init_window_once
sets Vframe_list to Qnil. After calling make_initial_frame, Vframe_list
becomes non-nil, so !NILP (Vframe_list) means init_window_once was
passed.

Dmitry



[-- Attachment #2: window_init.patch --]
[-- Type: text/plain, Size: 1614 bytes --]

=== modified file 'src/frame.c'
--- src/frame.c	2012-06-26 14:41:01 +0000
+++ src/frame.c	2012-06-27 17:55:27 +0000
@@ -465,10 +465,7 @@
   Lisp_Object frame;
 
   eassert (initial_kboard);
-
-  /* The first call must initialize Vframe_list.  */
-  if (! (NILP (Vframe_list) || CONSP (Vframe_list)))
-    Vframe_list = Qnil;
+  eassert (NILP (Vframe_list));
 
   terminal = init_initial_terminal ();
 

=== modified file 'src/window.c'
--- src/window.c	2012-06-26 14:41:01 +0000
+++ src/window.c	2012-06-27 17:53:02 +0000
@@ -117,9 +117,6 @@
 /* Incremented for each window created.  */
 static int sequence_number;
 
-/* Nonzero after init_window_once has finished.  */
-static int window_initialized;
-
 /* Hook to run when window config changes.  */
 static Lisp_Object Qwindow_configuration_change_hook;
 
@@ -3019,9 +3016,7 @@
   windows_or_buffers_changed++;
 
   /* We must select BUFFER for running the window-scroll-functions.  */
-  /* We can't check ! NILP (Vwindow_scroll_functions) here
-     because that might itself be a local variable.  */
-  if (window_initialized)
+  if (!NILP (Vframe_list))
     {
       record_unwind_protect (Fset_buffer, Fcurrent_buffer ());
       Fset_buffer (buffer);
@@ -6472,14 +6467,15 @@
 void
 init_window_once (void)
 {
-  struct frame *f = make_initial_frame ();
+  struct frame *f;
+
+  Vframe_list = Qnil;
+  f = make_initial_frame ();
   XSETFRAME (selected_frame, f);
   Vterminal_frame = selected_frame;
   minibuf_window = f->minibuffer_window;
   selected_window = f->selected_window;
   last_nonminibuf_frame = f;
-
-  window_initialized = 1;
 }
 
 void

Re: 'struct window' cleanup #3

[Eli Zaretskii]

> Date: Wed, 27 Jun 2012 21:59:58 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> I would like to propose alternate redesign for this: init_window_once
> sets Vframe_list to Qnil. After calling make_initial_frame, Vframe_list
> becomes non-nil, so !NILP (Vframe_list) means init_window_once was
> passed.

How is this better than the current use of window_initialized?  At
least that flag is clearly meant to indicate that window.c has been
initialized.  By contrast, if we use the fact that Vframe_list is
initialized in make_initial_frame that is called by init_window_once,
we are piggy-backing a mechanism that was never meant to be such an
indication.

Re: 'struct window' cleanup #3

[Dmitry Antipov]

On 06/27/2012 09:24 PM, Eli Zaretskii wrote:

>> 3. The check:
>>
>>      && INTEGERP (w->window_end_vpos)
>>      && XFASTINT (w->window_end_vpos) < w->current_matrix->nrows
>>
>>      is transformed to:
>>
>>      && w->window_end_vpos > 0
>>      && w->window_end_vpos < w->current_matrix->nrows
>
> I don't think this is right.  window_end_vpos is the number of the
> last glyph row of a glyph matrix, so it can legitimately be zero, when
> w->current_matrix->nrows is 1.  We need to initialize this member with
> some invalid value, like -1, if we want to distinguish between values
> we can and cannot trust.  Or maybe test window_end_valid here (to put
> our money where our mouth -- i.e. the commentary -- is ;-).
>
>>      I never get w->window_end_vpos >= w->current_matrix->nrows
>
> Try something that enlarges and then shrinks the minibuffer window.
> Or maybe split a window on a TTY.  Anyway, this does happen.
>
>>      but run into w->window_end_vpos == 0 several times.
>
> See above: it's a legitimate value.
>
>>      At this moment, I have no ideas whether Gerd was correct about
>>      fixing window.c, so this needs more detailed investigations.
>
> My gray hair taught me that Gerd is usually right.

I need more time to think on it. Nevertheless, I believe that other fields of
struct window (which aren't used in such a tricky way) may be converted
without a risk to break redisplay, and I did that in 108788 and 108790.

Dmitry

Re: 'struct window' cleanup #3

[Dmitry Antipov]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

One more minor simplification: W->region_showing may be only nil or t,
so it may be reduced to a bitfield.

Hard case:

   if (((!NILP (Vtransient_mark_mode)
	&& !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
        != !NILP (w->region_showing))
       || (!NILP (w->region_showing)
	  && !EQ (w->region_showing,
		  Fmarker_position (BVAR (XBUFFER (w->buffer), mark)))))

There are two possible cases:

   - both w->region_showing and Fmarker_position (BVAR (XBUFFER (w->buffer), mark))
     are nil;
   - w->region_showing is t and Fmarker_position (BVAR (XBUFFER (w->buffer), mark))
     is a number, so !EQ (...) is always non-zero.

So if-statement may be simplified to:

   if (((!NILP (Vtransient_mark_mode)
	&& !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
        != w->region_showing) || w->region_showing)

Dmitry

[-- Attachment #2: region_showing.patch --]
[-- Type: text/plain, Size: 3652 bytes --]

=== modified file 'src/window.h'
--- src/window.h	2012-06-29 11:48:08 +0000
+++ src/window.h	2012-07-01 09:55:44 +0000
@@ -198,10 +198,6 @@
        as long as the window shows that buffer.  */
     Lisp_Object base_line_pos;
 
-    /* If we have highlighted the region (or any part of it),
-       this is the mark position that we used, as an integer.  */
-    Lisp_Object region_showing;
-
     /* The column number currently displayed in this window's mode line,
        or nil if column numbers are not being displayed.  */
     Lisp_Object column_number_displayed;
@@ -289,6 +285,9 @@
        was last updated.  */
     unsigned last_had_star : 1;
 
+    /* Non-zero if we have highlighted the region (or any part of it).  */
+    unsigned region_showing : 1;
+
     /* Non-zero means current value of `start'
        was the beginning of a line when it was chosen.  */
     unsigned start_at_line_beg : 1;

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2012-06-29 18:52:54 +0000
+++ src/xdisp.c	2012-07-01 14:56:24 +0000
@@ -11202,7 +11202,7 @@
 	      != w->last_had_star)
 	  || ((!NILP (Vtransient_mark_mode)
 	       && !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
-	      != !NILP (w->region_showing)))
+	      != w->region_showing))
 	{
 	  struct buffer *prev = current_buffer;
 	  ptrdiff_t count = SPECPDL_INDEX ();
@@ -11400,7 +11400,7 @@
 	      != w->last_had_star)
 	  || ((!NILP (Vtransient_mark_mode)
 	       && !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
-	      != !NILP (w->region_showing)))
+	      != w->region_showing))
 	{
 	  struct buffer *prev = current_buffer;
 	  ptrdiff_t count = SPECPDL_INDEX ();
@@ -13105,10 +13105,7 @@
      the optimization directly below this if-statement.  */
   if (((!NILP (Vtransient_mark_mode)
 	&& !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
-       != !NILP (w->region_showing))
-      || (!NILP (w->region_showing)
-	  && !EQ (w->region_showing,
-		  Fmarker_position (BVAR (XBUFFER (w->buffer), mark)))))
+       != w->region_showing) || w->region_showing)
     CHARPOS (this_line_start_pos) = 0;
 
   /* Optimize the case that only the line containing the cursor in the
@@ -13276,7 +13273,7 @@
 	       && (EQ (selected_window,
 		       BVAR (current_buffer, last_selected_window))
 		   || highlight_nonselected_windows)
-	       && NILP (w->region_showing)
+	       && !w->region_showing
 	       && NILP (Vshow_trailing_whitespace)
 	       && !cursor_in_echo_area)
 	{
@@ -14953,7 +14950,7 @@
          set the cursor.  */
       && !(!NILP (Vtransient_mark_mode)
 	   && !NILP (BVAR (current_buffer, mark_active)))
-      && NILP (w->region_showing)
+      && !w->region_showing
       && NILP (Vshow_trailing_whitespace)
       /* This code is not used for mini-buffer for the sake of the case
 	 of redisplaying to replace an echo area message; since in
@@ -16324,7 +16321,7 @@
   /* Can't do this if region may have changed.  */
   if ((!NILP (Vtransient_mark_mode)
        && !NILP (BVAR (current_buffer, mark_active)))
-      || !NILP (w->region_showing)
+      || w->region_showing
       || !NILP (Vshow_trailing_whitespace))
     return 0;
 
@@ -17156,7 +17153,7 @@
     GIVE_UP (11);
 
   /* Likewise if showing a region.  */
-  if (!NILP (w->region_showing))
+  if (w->region_showing)
     GIVE_UP (10);
 
   /* Can't use this if overlay arrow position and/or string have
@@ -19134,7 +19131,7 @@
     }
 
   /* Is IT->w showing the region?  */
-  it->w->region_showing = it->region_beg_charpos > 0 ? Qt : Qnil;
+  it->w->region_showing = it->region_beg_charpos > 0;
 
   /* Clear the result glyph row and enable it.  */
   prepare_desired_row (row);

Re: 'struct window' cleanup #3

[Andreas Schwab]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> So if-statement may be simplified to:
>
>   if (((!NILP (Vtransient_mark_mode)
> 	&& !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
>        != w->region_showing) || w->region_showing)

Which is the same as:

    if (w->region_showing
        || (!NILP (Vtransient_mark_mode)
            && !NILP (BVAR (XBUFFER (w->buffer), mark_active))))

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."