From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: 'struct window' cleanup #2 Date: Mon, 25 Jun 2012 19:39:40 +0300 Message-ID: <83sjdj9whf.fsf@gnu.org> References: <4FE827B6.6020306@yandex.ru> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: dough.gmane.org 1340642387 9373 80.91.229.3 (25 Jun 2012 16:39:47 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Mon, 25 Jun 2012 16:39:47 +0000 (UTC) Cc: emacs-devel@gnu.org To: Dmitry Antipov Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Jun 25 18:39:47 2012 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1SjCKI-0004TE-ES for ged-emacs-devel@m.gmane.org; Mon, 25 Jun 2012 18:39:46 +0200 Original-Received: from localhost ([::1]:39938 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SjCKI-0006ah-Dy for ged-emacs-devel@m.gmane.org; Mon, 25 Jun 2012 12:39:46 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:45840) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SjCKF-0006a6-T2 for emacs-devel@gnu.org; Mon, 25 Jun 2012 12:39:45 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SjCK8-0007KF-EA for emacs-devel@gnu.org; Mon, 25 Jun 2012 12:39:43 -0400 Original-Received: from mtaout21.012.net.il ([80.179.55.169]:53914) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SjCK8-0007JW-5V for emacs-devel@gnu.org; Mon, 25 Jun 2012 12:39:36 -0400 Original-Received: from conversion-daemon.a-mtaout21.012.net.il by a-mtaout21.012.net.il (HyperSendmail v2007.08) id <0M6600A00LPLF800@a-mtaout21.012.net.il> for emacs-devel@gnu.org; Mon, 25 Jun 2012 19:39:34 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([87.69.210.75]) by a-mtaout21.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0M6600AP1M9V7ZA0@a-mtaout21.012.net.il>; Mon, 25 Jun 2012 19:39:31 +0300 (IDT) In-reply-to: <4FE827B6.6020306@yandex.ru> X-012-Sender: halo1@inter.net.il X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-Received-From: 80.179.55.169 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:151159 Archived-At: > Date: Mon, 25 Jun 2012 12:56:22 +0400 > From: Dmitry Antipov > > - 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.