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

Gleb Smirnoff glebius at FreeBSD.org
Tue Feb 5 02:09:35 UTC 2013


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.


More information about the freebsd-bugs mailing list