svn commit: r295190 - user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools

[Bruce Evans]

On Wed, 3 Feb 2016, Garrett Cooper wrote:

> Log:
>  Don't test for tc < 0 in snmp_oct2tc(..), snmp_tc2oid(..), and
>  snmp_tc2oct(..) to mute valid -Wtautological-compare warnings as
>  tc is an enum and (by the current definition of the enum) will
>  always be positive

It is too easy to break warnings by unimproving the code.

> Modified: user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools/bsnmptc.c
> ==============================================================================
> --- user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools/bsnmptc.c	Wed Feb  3 02:02:01 2016	(r295189)
> +++ user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools/bsnmptc.c	Wed Feb  3 02:03:00 2016	(r295190)
> @@ -158,7 +158,7 @@ snmp_oct2tc(enum snmp_tc tc, uint32_t le
> 	uint32_t tc_len;
> 	char * buf;
>
> -	if (tc < 0 || tc > SNMP_UNKNOWN)
> +	if (tc > SNMP_UNKNOWN)
> 		tc = SNMP_UNKNOWN;
>
> 	if (text_convs[tc].len > 0)

The type of an enum is implementation-dependent.  Another compiler might
implement this enum as a signed integer.  Then buggy callers might pass an
invalid negative value in tc.  The old code checks for this.

Also, enum constants have type int.  If the enum type is unsigned and the
warnings are too fussy then they would complain about comparison between
the unsigned tc and the signed SNMP_UNKNOWN.

The enum values are just 0-11, with SNMP_UNKNOWN = 11 last.  The above
check depends on the range being contiguous.  The type int is good for
representing this range, so the type of the enum is quite likely to be
an int.  Probably it is actually unsigned char.  unsigned int would
be a bad choice since it tends to give (uns)sign extension bugs and
doesn't save any space.  The upper range of an unsigned int is unusable
for enums since enum values have type int.  But signed char would work
here too.

Similary for typedefed types.  They typedefed type might be unsigned
in one implementation and signed in another.  You don't want code that
carefully checks for values < 0 to be broken to mute compiler warnings.
Similarly if the check is for <= UCHAR_MAX and this happens to be
tautological in one implementation where the type is unsigned char.

Warning for out-of-range enum values is clearly broken.  In another
use, the top value might be the limit for the type (most likely if
the range is 0-255 and the enum type is unsigned char).  You don't
want a warning about tc >= 256 being generated just because the top
value happens to be the limit of the type.

Bruce