[ovs-dev] [PATCH] ofp-meter: Fix ds_put_format that treats enum type as short integer

Yifeng Sun pkusunyifeng at gmail.com
Wed Jun 27 02:13:33 UTC 2018


Thanks for the clarification. The document about enum type
is quite confusing and it is difficult to get it right.

How about use %x plus a cast to unsigned? I think a hex output
is more intuitive for flags, what do you think?

Best,
Yifeng

On Tue, Jun 26, 2018 at 6:16 PM, Ben Pfaff <blp at ovn.org> wrote:

> On Tue, Jun 26, 2018 at 10:30:30AM -0700, Yifeng Sun wrote:
> > Travis job fails because of the below error:
> >
> > lib/ofp-meter.c:340:48: error: format specifies type 'unsigned short'
> > but the argument has underlying type 'unsigned int' [-Werror,-Wformat]
> >         ds_put_format(s, "flags:0x%"PRIx16" ", flags);
> >
> > This patch fixes it by treating enum type as int type.
> > (6.7.2.2 Enumerationspecifiers)
>
> Thank you for the patch.
>
> This citation from C99 looks like a misunderstanding.  In standard C
> (all editions), the members of an enumeration (the constants) have type
> int, as paragraph 3 says:
>
>     The identifiers in an enumerator list are declared as constants that
>     have type int and may appear wherever such are permitted.
>
> However, the enumeration type itself is not necessarily int.  Instead,
> paragraph 4 says:
>
>     Each enumerated type shall be compatible with char, a signed integer
>     type, or an unsigned integer type. The choice of type is
>     implementation-defined...
>
> This means that, from a standards perspective, which is the correct type
> here will differ from one implementation to another.  It's not even
> necessarily a type that promotes to 'int' (it could be "unsigned int" or
> even wider than int).  It's a weird choice but that's what the standard
> says.
>
> The particular patch here doesn't seem to match up to the description,
> either.  To treat them as int, one would want to use %x, since that's
> the format specifier for (unsigned) int, but this uses PRIx32, which
> isn't necessarily for int (it could be for long int, for example).
>
> >      if (flags) {
> > -        ds_put_format(s, "flags:0x%"PRIx16" ", flags);
> > +        ds_put_format(s, "flags:0x%"PRIx32" ", flags);
> >      }
>
> I think that a correct fix here would be to use %u plus a cast to
> unsigned.
>
> Thanks,
>
> Ben.
>


More information about the dev mailing list