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: Intervals crash Date: Fri, 24 Sep 2010 12:31:22 +0200 Message-ID: <83d3s3toud.fsf@gnu.org> References: <87aan82uar.fsf@stupidchicken.com> Reply-To: Eli Zaretskii NNTP-Posting-Host: lo.gmane.org X-Trace: dough.gmane.org 1285324319 5044 80.91.229.12 (24 Sep 2010 10:31:59 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 24 Sep 2010 10:31:59 +0000 (UTC) Cc: emacs-devel@gnu.org To: Chong Yidong Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Sep 24 12:31:57 2010 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Oz5ZJ-00079l-E1 for ged-emacs-devel@m.gmane.org; Fri, 24 Sep 2010 12:31:57 +0200 Original-Received: from localhost ([127.0.0.1]:60001 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Oz5Z1-0002YX-3m for ged-emacs-devel@m.gmane.org; Fri, 24 Sep 2010 06:31:35 -0400 Original-Received: from [140.186.70.92] (port=37822 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Oz5Yn-0002VZ-47 for emacs-devel@gnu.org; Fri, 24 Sep 2010 06:31:27 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Oz5Yl-0004J2-8B for emacs-devel@gnu.org; Fri, 24 Sep 2010 06:31:20 -0400 Original-Received: from mtaout22.012.net.il ([80.179.55.172]:46659) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Oz5Yk-0004IZ-Ul for emacs-devel@gnu.org; Fri, 24 Sep 2010 06:31:19 -0400 Original-Received: from conversion-daemon.a-mtaout22.012.net.il by a-mtaout22.012.net.il (HyperSendmail v2007.08) id <0L9800H00YHQDQ00@a-mtaout22.012.net.il> for emacs-devel@gnu.org; Fri, 24 Sep 2010 12:31:17 +0200 (IST) Original-Received: from HOME-C4E4A596F7 ([77.127.203.3]) by a-mtaout22.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0L9800G9YYK4J8C0@a-mtaout22.012.net.il>; Fri, 24 Sep 2010 12:31:17 +0200 (IST) In-reply-to: <87aan82uar.fsf@stupidchicken.com> X-012-Sender: halo1@inter.net.il X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:130744 Archived-At: > From: Chong Yidong > Date: Thu, 23 Sep 2010 14:23:24 -0400 > Cc: emacs-devel@gnu.org > > The LENGTH macro in intervals.h:114 has to be able to return a negative > number. Given this information, I find it difficult to reconcile the facts with the code. The implementation does this: struct interval { /* The first group of entries deal with the tree structure. */ EMACS_UINT total_length; /* Length of myself and both children. */ EMACS_UINT position; /* Cache of interval's character position. */ ... /* The total size of all text represented by this interval and all its children in the tree. This is zero if the interval is null. */ #define TOTAL_LENGTH(i) ((i) == NULL_INTERVAL ? 0 : (i)->total_length) /* The size of text represented by this interval alone. */ #define LENGTH(i) ((i) == NULL_INTERVAL ? 0 : (TOTAL_LENGTH ((i)) \ - TOTAL_LENGTH ((i)->right) \ - TOTAL_LENGTH ((i)->left))) If total_length is an unsigned type, then computing negative values from unsigned values is asking for trouble, no? The above definitions led me to believe that using EMACS_UINT for the result of LENGTH would be safe, because, I reasoned, no one in their right minds would otherwise use an unsigned type. Also, the comment in the code that crashes: EMACS_INT amt = LENGTH (i); if (amt > 0) /* Only used on zero-length intervals now. */ abort (); lies through its teeth, doesn't it? It implies that the result of LENGTH is always non-negative, which again convinced me that my understanding is correct. But the crash says otherwise. In addition, the commentary to delete_interval says: /* Delete interval I from its tree by calling `delete_node' and properly connecting the resultant subtree. I is presumed to be empty; that is, no adjustments are made for the length of I. */ "I is presumed to be empty." Is that also a lie? Or is the code buggy, and using an unsigned type simply exposed those bugs? If the code is correct and the comments are wrong, I'd say we should fix the comments, and make total_length a signed type (EMACS_INT). Opinions and insights are most welcome.