Date: Tue, 5 Feb 2013 21:19:53 +1100 (EST)
From: Bruce Evans <brde@optusnet.com.au>
To: Gleb Smirnoff <glebius@FreeBSD.org>
Cc: freebsd-bugs@FreeBSD.org, Bruce Evans <brde@optusnet.com.au>
Subject: Re: kern/175759: Correct data types for fields of struct qm_trace{} from <sys/queue.h>
Message-ID: <20130205210237.P1500@besplex.bde.org>
In-Reply-To: <20130205020931.GY91075@glebius.int.ru>
References: <201302041420.r14EK4IP068305@freefall.freebsd.org> <20130205024617.X3554@besplex.bde.org> <20130205020931.GY91075@glebius.int.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 5 Feb 2013, Gleb Smirnoff wrote:
> 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.
Using int would only be a style bug, since FreeBSD has thousands if
not millions of other assumptions that ints are precisely 32 bits. Anyway,
int32_t is large enough to hold 2**31-1.
> 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.
I used to prefer the latter, but know better now :-).
__LINE__ constant literals probably have type int or long, so it is
inconsistent to store them as unsigned. But I can't think of any
useful expression where the behaviour would be different due to not
being unsigned -- the expression (p1->lastline - p2->lastline) might
be useful (if unsigned is not used to break it), but there is no
similar expression with 2 __LINE__ constants.
Bruce
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130205210237.P1500>