Date: Tue, 5 Feb 2013 06:09:31 +0400
From: Gleb Smirnoff <glebius@FreeBSD.org>
To: Bruce Evans <brde@optusnet.com.au>
Cc: freebsd-bugs@freebsd.org
Subject: Re: kern/175759: Correct data types for fields of struct qm_trace{} from <sys/queue.h>
Message-ID: <20130205020931.GY91075@glebius.int.ru>
In-Reply-To: <20130205024617.X3554@besplex.bde.org>
References: <201302041420.r14EK4IP068305@freefall.freebsd.org> <20130205024617.X3554@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130205020931.GY91075>