svn commit: r314989 - head/usr.bin/vmstat

[Ngie Cooper]

> On Mar 10, 2017, at 03:59, Pedro Giffuni <pfg at FreeBSD.org> wrote:
> 
>> On 3/10/2017 2:45 AM, Bruce Evans wrote:
>>> 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.
>> 
> 
> I haven't looked at the code but it would seem like you can unsign c and avoid the cast.

Yeah. This might introduce a domino effect though of changes.
Cheers!
-Ngie