add warnings enum-to-int and int-to-enum

Well, let's go for the second iteration...

On Wed September 2 2009 01:24:33 Josh Triplett wrote:
> warnings:
> > static void foo(void) {
> >     enum ENUM_TYPE_A { VALUE_A } var_a;
> >     enum ENUM_TYPE_B { VALUE_B } var_b;
> >     enum /* anon. */ { VALUE_C } anon_enum_var;
> >     int i;
> >
> >     // always OK
> >     var_a = VALUE_A;
> >     var_a = (enum ENUM_TYPE_A) VALUE_B;
> >     var_b = (enum ENUM_TYPE_B) i;
> >     i = (int) VALUE_A;
>
> Fair enough; a cast should indeed make the warning go away.  Good catch
> to include the case of casting an enum value to a different enum type.
> I'd also suggest including some casts of values like 0 in the "always
> OK" section:
>
>     var_a = (enum ENUM_TYPE_B) 0;

This is IMO not "always OK":
warning: mixing different enum types
    int enum ENUM_TYPE_B  versus
    int enum ENUM_TYPE_A

Why should we have an exception for zero? Then we would not distinguish 
between VALUE_A and VALUE_B? This needs some justification. Have you 
considered the case that zero is even not valid at all for an enum?

> >     i = VALUE_B;
>
> Why does this not generate a warning with Wenum-to-int?  You should have
> to cast VALUE_B to int.

I was not sure if this is actually useful ... now it does.

> >     // caught by -Wint-to-enum (default)
> >     var_a = VALUE_B;
> >     var_b = VALUE_C;
> >     anon_enum_var = VALUE_A;
>
> Why does Wenum-mismatch not catch these?  Because it treats those
> constants as ints?  Regardless of the cause, this seems wrong.  If you
> explicitly declare a variable with enum type, assigning the wrong enum
> constant to it should generate a enum-mismatch warning, not an

Well, now caught by -Wenum-mismatch instead.

> int-to-enum warning.  However, I understand if this proves hard to fix,
> and generating *some* warning seems like an improvement over the current
> behavior of generating *no* warning.

Nope, it's easy to fix if you don't care about the change in behavior
of -Wenum-mismatch.

> >     var_a = 0;
> >     var_b = i;
> >     anon_enum_var = 0;
> >     anon_enum_var = i;
>
> I'd also suggest including tests with enum constants casted to integers:
>
>     var_a = (int) VALUE_A;
>     var_a = (int) VALUE_B;

Added.

> > --- a/sparse.1
> > +++ b/sparse.1
> > @@ -149,6 +149,18 @@ Sparse issues these warnings by default.  To turn
> > them off, use \fB\-Wno\-enum\-mismatch\fR.
> >  .
> >  .TP
> > +.B \-Wenum\-to\-int
> > +Warn about casting of an \fBenum\fR type to int and thus possible lost
> > of +information about the type.
>
> This should not say "warn about casting", because it specifically
> *doesn't* warn about casting.  s/casting of/converting/.
>
> I don't think "possible loss of information" seems like the reason to
> care about this warning.  These warnings just represent ways of getting
> sparse to make you treat enums as distinct types from int.  I'd suggest
> dropping the second half of the sentence.
>
> I'd also suggest adding something like "An explicit cast to int will
> suppress this warning.".

Fixed.

> > +.TP
> > +.B \-Wint\-to\-enum
> > +Warn about casting of int (or incompatible enumeration constant) to
> > \fBenum\fR.
>
> OK, so the test represents *documented* behavior, but it still doesn't
> seem right. :)  The "(or incompatible enumeration constant)" bit seems
> like it should become part of Wenum-mismatch.
>
> Or if necessary, perhaps part of some new flag, like
> Wenum-constant-mismatch, but that seems like overkill.
>
> s/casting of/converting/, as above.
>
> I'd also suggest adding "An explicit cast to the enum type will suppress
> this warning.".

Fixed.

I would really appreciate some native (or native like) speaker willing to 
reword my man page attempts completely. Any volunteer around? :-)

On Wed September 2 2009 02:27:08 Stephen Hemminger wrote:
> there is lots of code that does:
>
> enum {
>    my_register_1   = 0x001,
>    my_register_2   = 0x002,
> };
>
>
> foo(void) {
>    write_register(my_register_1, SETUP_VAL_X);
> ...
>
> So going from enum to int must be optional, but int to enum should
> trigger a warning.

This should be already working. If this is not the case, please write me
an minimal example along with the expected result. I'll take care of it...

> I'll give it a pass on the kernel for giggles.

Great!

Kamil

add warnings enum-to-int and int-to-enum

Commit Message

Comments

Patch