On Tue, 1 Sep 2009 16:24:33 -0700
Josh Triplett <josh@xxxxxxxxxxxxxxxx> wrote:
> On Tue, Sep 01, 2009 at 11:59:09PM +0200, Kamil Dudka wrote:
> > On Monday 31 of August 2009 22:53:54 Josh Triplett wrote:
> > > That seems quite sensible, and it even seems like something sparse
> > > should warn about by default.
> > >
> > > The reverse, warning about the assignment of an enum value to int, seems
> > > useful as well, but sparse shouldn't issue that warning by default
> > > because a lot of code just uses enum to define constants.
> > >
> > > Distinguishing between anonymous and named enums seems useful as well.
> > > An anonymous enum just creates some named constants but doesn't create a
> > > type to use them with, so assigning those constants to int shouldn't
> > > generate a warning. (Corner case: "enum { ... } foo;".)
> >
> > Here is my first take at casting from/to enum along with a simple test case.
> > Any feedback welcome...
>
> Thanks for writing the patch. This looks really good so far. I have a
> couple of comments on the cases you warn about and how you classify the
> 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;
>
> > i = VALUE_B;
>
> Why does this not generate a warning with Wenum-to-int? You should have
> to cast VALUE_B to int.
>
> > anon_enum_var = VALUE_C;
> > i = VALUE_C;
> > i = anon_enum_var;
>
> Excellent, this represents exactly the behavior I had in mind when I
> suggested the anonymous enum issue.
>
> > // already caught by -Wenum-mismatch (default)
> > var_a = var_b;
> > var_b = anon_enum_var;
> > anon_enum_var = var_a;
> >
> > // 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
> 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.
>
> > 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;
>
> > // caught only with -Wenum-to-int
> > i = var_a;
> > }
>
> > --- 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.".
>
> > +.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.".
>
> - Josh Triplett
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.
I'll give it a pass on the kernel for giggles.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
