From owner-freebsd-bugs@FreeBSD.ORG Mon Feb 4 16:24:02 2013 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 7423B400; Mon, 4 Feb 2013 16:24:02 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx08.syd.optusnet.com.au (fallbackmx08.syd.optusnet.com.au [211.29.132.10]) by mx1.freebsd.org (Postfix) with ESMTP id 13EBD766; Mon, 4 Feb 2013 16:24:01 +0000 (UTC) Received: from mail35.syd.optusnet.com.au (mail35.syd.optusnet.com.au [211.29.133.51]) by fallbackmx08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r14GNrdI006363; Tue, 5 Feb 2013 03:23:53 +1100 Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail35.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r14GNi6m027145 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 5 Feb 2013 03:23:46 +1100 Date: Tue, 5 Feb 2013 03:23:44 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Gleb Smirnoff Subject: Re: kern/175759: Correct data types for fields of struct qm_trace{} from In-Reply-To: <201302041420.r14EK4IP068305@freefall.freebsd.org> Message-ID: <20130205024617.X3554@besplex.bde.org> References: <201302041420.r14EK4IP068305@freefall.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=evCHVfVX c=1 sm=1 a=Mvik2ojQbM4A:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=HXKBcPQmqRAA:10 a=OtbnSwKdCio8T-pgMJcA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 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: Mon, 04 Feb 2013 16:24:02 -0000 On Mon, 4 Feb 2013, Gleb Smirnoff wrote: > any additional comments for the attached patch. Is it ok from your > viewpoint? > Index: queue.h > =================================================================== > --- queue.h (revision 245741) > +++ queue.h (working copy) > @@ -105,13 +105,14 @@ > #ifdef QUEUE_MACRO_DEBUG > /* Store the last 2 places the queue element or head was altered */ > struct qm_trace { > - char * lastfile; > - int lastline; > - char * prevfile; > - int prevline; > + const char * lastfile; > + unsigned long lastline; > + const char * prevfile; > + unsigned long prevline; > }; Unsigned long is unnecessarily large. It wastes space on 64-bit arches. The change doesn't change the wastage, because space was already wasted on 64-bit arches by mispacking the struct (with unnamed padding after the ints). It changes the API unnecessarily by changing signed variables to unsigned. Sign variables are easier to use, and changing to unsigned ones risks sign extension bugs. According to your quote of the C standard, int32_t is enough. (I couldn't find anything directly about either the type or limit of __LINE__ in the n869.txt draft of C99, but #line is limited to 2**31-1. n1124.pdf says much the same, except it says that __LINE__ is an integer constant where n869.txt says that __LINE__ is a decimal constant. Both of these seem to be wrong -- "decimal constants" include floating point ones, and "integer constants" include octal and hex ones.) The change preserves many style bugs: - no prefix in member names - member names not sorted inversely on size. This gives the space wastage. - member names not indented - space between '*' and member names. Bruce