kern/175759: Correct data types for fields of struct qm_trace{} from <sys/queue.h>

Bruce Evans brde at optusnet.com.au
Tue Feb 5 10:20:05 UTC 2013


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


More information about the freebsd-bugs mailing list