Our testing needs work...

[Eric S. Raymond]

Hal Murray <hmurray at megapathdsl.net>: 
> Would it help to understand the code if we used something conspicuous to mark 
> casts that are there only to suppress warnings?  Is there already a common 
> way to do that?  I'm thinking of something like INT, UINT, LONG and ULONG.

There is no common way to do that.

Generally, when I read code like this, I find it safe to assume that
any cast of a numeric type (as opposed to a pointer) is there to
suppress a warning. There are only a few well-defined exceptions, such
as int casts to truncate floats and some tricks used for bit vectors.
Thus I don't think these need to be marked.

> [Still some warnings...]
> 
> > I'd like to fix those. Can you send me a transcript?  I'm sure I could
> > squash at least a few.
> 
> Thanks.  There aren't all that many.
> 
> There is one nasty case.  I sent a message about it a few days ago.
> 
> ../ntpd/ntp_io.c:4629:7: warning: comparison between signed and unsigned 
> integer expressions [-Wsign-compare]
> 
> The problem is a glitch/bug in a system macro: NLMSG_OK.  Older compilers 
> complain.  I don't think we can fix it on our end.  Inside, there are 3 
> tests.  If we fix one we break another.

Agreed.

> We could clone the macro and fix our copy.  That turns into a maintainance 
> hazard, but I don't think that macro is likely to change.
> 
> We could live with it: add a comment to the code and another wherever we 
> claim (almost) no warnings left.

I'm for living with it.

> I think these are the others:
> 
> [135/217] Compiling ntpd/refclock_acts.c
> ../ntpd/refclock_acts.c: In function 'acts_message':
> ../ntpd/refclock_acts.c:384:6: warning: array subscript has type 'char' 
> [-Wchar-subscripts]
>       if (isspace(*cp))
> 
> [145/217] Compiling ntpd/refclock_jjy.c
> ../ntpd/refclock_jjy.c: In function 'jjy_receive':
> ../ntpd/refclock_jjy.c:664:5: warning: array subscript has type 'char' 
> [-Wchar-subscripts]
>      if ( ! iscntrl( up->sRawBuf[i] ) ) {
>      ^
> ../ntpd/refclock_jjy.c: In function 'jjy_start_telephone':
> ../ntpd/refclock_jjy.c:2694:3: warning: array subscript has type 'char' 
> [-Wchar-subscripts]
>    if ( isdigit( *(sys_phone[0]+i) ) ) {
> 
> [167/217] Compiling ntpq/ntpq.c
> ../ntpq/ntpq.c: In function 'list_md_fn':
> ../ntpq/ntpq.c:3381:2: warning: array subscript has type 'char' 
> [-Wchar-subscripts]
>   if( islower(*cp) )

This kind of warning has come up before. These could be fixed with int
casts, but I'm reluctant to do that because I think the compiler is
being insufferably pedantic here and I'd prefer to find a way to
shut it up.

Here is what's happening.  The ctype functions formally take an int, but
semantically take a char.  They're implemented by a macro that indexes
a hidden array.  The compiler is complaining because the array is being
indexed by an expression of non-integral type.  Which, in this case,
is silly.

There's probably an option to suppress this check that defaults on but
your one build environment has inverted.  It would be nice to figure
out what that is.

> 
> [ 66/223] Compiling libparse/gpstolfp.c
> ../libparse/gpstolfp.c: In function ‘gpstolfp’:
> ../libparse/gpstolfp.c:26:3: warning: this decimal constant is unsigned only 
> in ISO C90 [enabled by default]

The line is:

  lfp->l_ui = (uint32_t)(weeks * SECSPERWEEK + days * SECSPERDAY + seconds + GPSORIGIN); /* convert to NTP time */

The relevant declarations are in include/ntp_calendar.h:

#define	SECSPERMIN	(60)
#define	MINSPERHR	(60)
#define	HRSPERDAY	(24)
#define	DAYSPERWEEK	(7)
#define	DAYSPERYEAR	(365)
#define	SECSPERHR	(SECSPERMIN * MINSPERHR)
#define	SECSPERDAY	(SECSPERHR * HRSPERDAY)
#define	SECSPERWEEK	(DAYSPERWEEK * SECSPERDAY)
#define	SECSPERYEAR	(365 * SECSPERDAY)
#define GPSORIGIN       2524953600

Nothing jumps out at me here.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>