Date: Wed, 3 Feb 2016 18:46:41 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <ngie@freebsd.org> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r295190 - user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools Message-ID: <20160203182332.Y898@besplex.bde.org> In-Reply-To: <201602030203.u13230B6054691@repo.freebsd.org> References: <201602030203.u13230B6054691@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160203182332.Y898>