Date: Tue, 5 Oct 2010 07:00:12 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Poul-Henning Kamp <phk@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r213401 - head/sys/sys Message-ID: <20101005065627.S1603@besplex.bde.org> In-Reply-To: <201010041048.o94Amlao010438@svn.freebsd.org> References: <201010041048.o94Amlao010438@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 4 Oct 2010, Poul-Henning Kamp wrote:
> Log:
> Certain static code analysis tools (FlexeLint being one) are very
> suspicious about 'l' and '1' being confused in numeric constants.
> The fear being that some old fart programmer might still think that
> he is using a Remmington Noiseless as input terminal device.
>
> An easy way to placate this fear is to use capital 'L' or to put
> the 'u' in unsigned constants in front of the 'l'.
A better way would be to fix the benign type mismatches and style bugs
in this code. "long long" shouldn't exist, so all uses of it are style
bugs at best, so "ll" suffixes are style bugs at best. Here they are
also type mismatches.
> Modified:
> head/sys/sys/time.h
>
> Modified: head/sys/sys/time.h
> ==============================================================================
> --- head/sys/sys/time.h Mon Oct 4 07:00:47 2010 (r213400)
> +++ head/sys/sys/time.h Mon Oct 4 10:48:47 2010 (r213401)
> @@ -95,11 +95,11 @@ bintime_mul(struct bintime *bt, u_int x)
> {
> uint64_t p1, p2;
>
> - p1 = (bt->frac & 0xffffffffllu) * x;
> + p1 = (bt->frac & 0xffffffffull) * x;
> p2 = (bt->frac >> 32) * x + (p1 >> 32);
> bt->sec *= x;
> bt->sec += (p2 >> 32);
> - bt->frac = (p2 << 32) | (p1 & 0xffffffffllu);
> + bt->frac = (p2 << 32) | (p1 & 0xffffffffull);
> }
>
> #define bintime_clear(a) ((a)->sec = (a)->frac = 0)
>
Type mismatches: there are no long longs here; there are only uint64_t's,
which may be shorter than unsigned long long. The uint64_t's here are:
- p1 and p2 declared explicitly above:
- bt_frac^Wfrac is uint64_t
- bt_sec^Wsec is time_t, which is int32_t on some arches and int64_t on
others.
Also, the long long suffixes have no effect here, except for non-C
compilers that complain about 0xffffffff being larger than INT_MAX.
The constants are naturally ordinary unsigned int ones on all supported
arches (they equal to 2**32-1, and all supported arches have 32-bit
unsigned ints). Unsigned ints combine with uint64_t values in all
sub-expressions to give the correct a uint64_t result, so no further
promotions are needed to get the correct uint64_t result for the whole
expression. Older places in this file need to convert sub-expressions
in this file to uint64_t, and they do this using careful casts to
uint64_t, even starting with bogus long long constants which wouldn't
need to be cast if they were unsigned long long constants, even if
unsigbed long long were larger than uint64_t. These places spell
their long long mistakes as "LL" suffixes while the newer places
now spell them as "ull" suffixes.
I once policed this better, and had removed most long longs in the
kernel before C99 made the mistake of standardizing them. C99 also
standardized uint64_t, so there was even less need to use long long.
Fixes for older long longs in this file:
% Index: time.h
% ===================================================================
% RCS file: /home/ncvs/src/sys/sys/time.h,v
% retrieving revision 1.65
% diff -u -2 -r1.65 time.h
% --- time.h 7 Apr 2004 04:19:49 -0000 1.65
% +++ time.h 7 Apr 2004 11:28:54 -0000
% @@ -118,6 +118,5 @@
%
% bt->sec = ts->tv_sec;
% - /* 18446744073 = int(2^64 / 1000000000) */
% - bt->frac = ts->tv_nsec * (uint64_t)18446744073LL;
% + bt->frac = ts->tv_nsec * (((uint64_t)1 << 63) / (1000000000 >> 1));
% }
%
% @@ -135,6 +134,5 @@
%
% bt->sec = tv->tv_sec;
% - /* 18446744073709 = int(2^64 / 1000000) */
% - bt->frac = tv->tv_usec * (uint64_t)18446744073709LL;
% + bt->frac = tv->tv_usec * (((uint64_t)1 << 63) / (1000000 >> 1));
% }
% #endif /* __BSD_VISIBLE */
This also calculates 2**64 / 10**N in the correct way.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101005065627.S1603>