From owner-freebsd-bugs@FreeBSD.ORG Tue Feb 5 02:09:35 2013 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 62BD7C95 for ; Tue, 5 Feb 2013 02:09:35 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.69.10]) by mx1.freebsd.org (Postfix) with ESMTP id B0B90EA9 for ; Tue, 5 Feb 2013 02:09:33 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.6/8.14.6) with ESMTP id r1529VZj025011; Tue, 5 Feb 2013 06:09:31 +0400 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.6/8.14.6/Submit) id r1529V6T025010; Tue, 5 Feb 2013 06:09:31 +0400 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Tue, 5 Feb 2013 06:09:31 +0400 From: Gleb Smirnoff To: Bruce Evans Subject: Re: kern/175759: Correct data types for fields of struct qm_trace{} from Message-ID: <20130205020931.GY91075@glebius.int.ru> References: <201302041420.r14EK4IP068305@freefall.freebsd.org> <20130205024617.X3554@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline In-Reply-To: <20130205024617.X3554@besplex.bde.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-bugs@freebsd.org X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Feb 2013 02:09:35 -0000 On Tue, Feb 05, 2013 at 03:23:44AM +1100, Bruce Evans wrote: B> On Mon, 4 Feb 2013, Gleb Smirnoff wrote: B> B> > any additional comments for the attached patch. Is it ok from your B> > viewpoint? B> B> > Index: queue.h B> > =================================================================== B> > --- queue.h (revision 245741) B> > +++ queue.h (working copy) B> > @@ -105,13 +105,14 @@ B> > #ifdef QUEUE_MACRO_DEBUG B> > /* Store the last 2 places the queue element or head was altered */ B> > struct qm_trace { B> > - char * lastfile; B> > - int lastline; B> > - char * prevfile; B> > - int prevline; B> > + const char * lastfile; B> > + unsigned long lastline; B> > + const char * prevfile; B> > + unsigned long prevline; B> > }; B> B> Unsigned long is unnecessarily large. It wastes space on 64-bit B> arches. The change doesn't change the wastage, because space was B> already wasted on 64-bit arches by mispacking the struct (with B> unnamed padding after the ints). It changes the API unnecessarily B> by changing signed variables to unsigned. Sign variables are B> easier to use, and changing to unsigned ones risks sign extension B> bugs. B> B> According to your quote of the C standard, int32_t is enough. (I B> couldn't find anything directly about either the type or limit of B> __LINE__ in the n869.txt draft of C99, but #line is limited to 2**31-1. B> n1124.pdf says much the same, except it says that __LINE__ is an integer B> constant where n869.txt says that __LINE__ is a decimal constant. Both B> of these seem to be wrong -- "decimal constants" include floating point B> ones, and "integer constants" include octal and hex ones.) As Andrey pointed out, int may be smaller than 2**31-1, that's why longs are used. I know that you prefer signed variables since they are easier to use, but I prefer to explictily use unsigned in places where value can not go below zero by its definition. B> The change preserves many style bugs: B> - no prefix in member names B> - member names not sorted inversely on size. This gives the space B> wastage. B> - member names not indented B> - space between '*' and member names. Taken into account, thanks. -- Totus tuus, Glebius.