Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 10 Mar 2017 18:45:49 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Marcelo Araujo <araujo@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r314989 - head/usr.bin/vmstat
Message-ID:  <20170310181404.C1416@besplex.bde.org>
In-Reply-To: <201703100449.v2A4neTK046456@repo.freebsd.org>
References:  <201703100449.v2A4neTK046456@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 10 Mar 2017, Marcelo Araujo wrote:

> ...
> Log:
>  Use nitems() from sys/param.h and also remove the cast.
>
>  Reviewed by:	markj
>  MFC after:	3 weeks.
>  Differential Revision:	https://reviews.freebsd.org/D9937
> ...
> Modified: head/usr.bin/vmstat/vmstat.c
> ==============================================================================
> --- head/usr.bin/vmstat/vmstat.c	Fri Mar 10 04:30:31 2017	(r314988)
> +++ head/usr.bin/vmstat/vmstat.c	Fri Mar 10 04:49:40 2017	(r314989)
> @@ -288,17 +288,13 @@ retry_nlist:
> 				namelist[X_SUM].n_name = "_cnt";
> 				goto retry_nlist;
> 			}
> -			for (c = 0;
> -			     c < (int)(sizeof(namelist)/sizeof(namelist[0]));
> -			     c++)
> +			for (c = 0; c < nitems(namelist); c++)
> 				if (namelist[c].n_type == 0)
> 					bufsize += strlen(namelist[c].n_name) + 1;

This undoes fixes to compile at WARNS=2 in r87690 and now breaks at WARNS=3.
vmstat is still compiled at WARNS=1.

nitems suffers from the same unsigned poisoning as the sizeof() expression
(since it reduces to the same expression.  Casting to int in the expression
to fix the warning would break exotic cases.  Of course, nitems is
undocumented so no one knows when it is supposed to work).

vmstat compiles with no errors at WARNS=2.  At WARNS=3, it used to compile
with 9 excessive warnings (about the good style of omitting redundant
initializers).  Now it compiles with 10 excessive warnings.  1 more about
comparison between signed unsigned.  This warning is a compiler bug.  Both
gcc-4.2.1 and clang-3.9.0 have it.  It is enabled by -W, which is put in
CFLAGS at WARNS >= 3, or by -Wsign-compare.

These compilers even complain about:

 	int c;

 	for (c = 0; c < 1U; c++)
 		foo();

where it is extremely clear that c never gets converted to a wrong value
when it is promoted to unsigned for the comparison.  Compilers should
only warn about sign mismatches if they can't figure out the ranges or
if they can figure out the ranges but dangerous promotiions occur.
Compilers do excessive unrolling and other optimizations of loops like
the above, and they must figure out the ranges for this.

Compilers mostly err in the other direction by not warning when they
should, since average code has many sign mismatches and warning about
them all would give too many warnings.  Compilers have more excuses
for not being able to figure out the ranges in complicated expressions,
and until they are better at this so that they can avoid generating
spurious warnings, they are correct to never warn.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170310181404.C1416>