Date: Wed, 3 Feb 2016 11:07:27 -0600 From: Benjamin Kaduk <bjkfbsd@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Garrett Cooper <ngie@freebsd.org>, "src-committers@freebsd.org" <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: <CAJ5_RoBSChzCwR8NV2XT8GKzj2aYwv5CpGNavxBkYvRGH4bBew@mail.gmail.com> In-Reply-To: <20160203182332.Y898@besplex.bde.org> References: <201602030203.u13230B6054691@repo.freebsd.org> <20160203182332.Y898@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Feb 3, 2016 at 1:46 AM, Bruce Evans <brde@optusnet.com.au> wrote: > On Wed, 3 Feb 2016, Garrett Cooper wrote: > > >> 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 > By C, yes. The platform ABI for at least x86 specifies that enumerated types are represented as int (or maybe unsigned int if the values are too big; I didn't check). 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 > C99 brings in more rules about how values are laid out, so the range being contiguous is guaranteed unless someone did something "clever" in defining the enumerated values. (Values start at zero if not specified; value is one greater than the previous one if not specified, see 6.7.2.2 of n1256.pdf.) > 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. > > I think the compiler is being more clever than you indicate here -- it assumes as a given that the only values held by variables of the enumerated type are the specific enumerated values. This has nothing to do with the type used to hold the value, but rather that there are only 12 valid values, none of which are negative. (Some days I can convince myself that this is a too strict reading of the standard, but it seems to be what clang is interpreting the standard to be.) > 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. > > No argument here. -Ben > 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 > > _______________________________________________ > svn-src-user@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/svn-src-user > To unsubscribe, send any mail to "svn-src-user-unsubscribe@freebsd.org" >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ5_RoBSChzCwR8NV2XT8GKzj2aYwv5CpGNavxBkYvRGH4bBew>