diff mbox

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

Message ID 200909021353.17091.kdudka@redhat.com (mailing list archive)
State Rejected, archived
Headers show

Commit Message

Kamil Dudka Sept. 2, 2009, 11:53 a.m. UTC
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

Comments

Josh Triplett Sept. 2, 2009, 3:21 p.m. UTC | #1
On Wed, Sep 02, 2009 at 01:53:17PM +0200, Kamil Dudka wrote:
> 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?

Typo.  I meant ENUM_TYPE_A:

    var_a = (enum ENUM_TYPE_A) 0;

Hopefully that makes more sense as an "always OK" test. :)

Another crazy test for the "always OK" section:

    anon_enum_var = (__typeof__(anon_enum_var)) 0;
    anon_enum_var = (__typeof__(anon_enum_var)) VALUE_A;

> > >     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.

Thanks!  Yes, this seems very useful to me as a part of Wenum-to-int.

> > >     // 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.

Excellent!  This seems like fixing a deficiency in -Wenum-mismatch: it
should warn about using the wrong enum type with an enum value.  When
you use an enum type instead of int, you should use the *right* enum
type.  :)

Thanks for making this change.

> > >     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.

Thanks; this should help prevent strange corner-case regressions in the
future.

> > > --- 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? :-)

I tried to do so above; the changes I suggested (which you incorporated)
represented my attempt at rewording for clarity.

> +static void
> +warn_for_enum_to_int_cast (struct expression *expr, struct symbol *typeb)
> +{
[...]
> +}
> +
> +static void
> +warn_for_int_to_enum_cast (struct expression *expr, struct symbol *typeb)
> +{
[...]
> +}
> +
>  /*
>   * This gets called for implicit casts in assignments and
>   * integer promotion. We often want to try to move the
> @@ -267,7 +334,10 @@ static struct expression * cast_to(struct expression *old, struct symbol *type)
>  {
>  	struct expression *expr;
>  
> -	warn_for_different_enum_types (old->pos, old->ctype, type);
> +	warn_for_different_enum_types (old->pos, old->ctype, type,
> +				       old->enum_type);

At this point, it might make more sense to just pass old, rather than
three of its fields.  Would that work for the other callers of this
function?  In any case, that change can wait until after this patch.

> +	warn_for_enum_to_int_cast (old, type);
> +	warn_for_int_to_enum_cast (old, type);

I just realized that both of these functions need renaming as well:
s/cast/conversion/ or similar.  As with the manpage changes before,
"cast" describes the case it *doesn't* warn about.

> --- a/sparse.1
> +++ b/sparse.1
> @@ -149,6 +149,19 @@ Sparse issues these warnings by default.  To turn them off, use
>  \fB\-Wno\-enum\-mismatch\fR.
>  .
>  .TP
> +.B \-Wenum\-to\-int
> +Warn about converting an \fBenum\fR type to int. An explicit cast to int will
> +suppress this warning.
> +.
> +.TP
> +.B \-Wint\-to\-enum
> +Warn about converting int to \fBenum\fR type. An explicit cast to the right
> +\fBenum\fR type will suppress this warning.
> +
> +Sparse issues these warnings by default.  To turn them off, use
> +\fB\-Wno\-int\-to\-enum\fR.

This manpage text looks good to me.

> 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;
>     anon_enum_var = VALUE_C;
>     i = VALUE_C;
>     i = anon_enum_var;
>     i = 7;
> 
>     // caught by -Wenum-mismatch (default) even without the patch
>     var_a = var_b;
>     var_b = anon_enum_var;
>     anon_enum_var = var_a;
>     var_a = (enum ENUM_TYPE_B) 0;
> 
>     // caught by -Wenum-mismatch (default) only with the patch applied
>     var_a = VALUE_B;
>     var_b = VALUE_C;
>     anon_enum_var = VALUE_A;
> 
>     // caught by -Wint-to-enum (default)
>     var_a = 0;
>     var_b = i;
>     anon_enum_var = 0;
>     anon_enum_var = i;
>     var_a = (int) VALUE_A;
>     var_a = (int) VALUE_B;
> 
>     // caught only with -Wenum-to-int
>     i = var_a;
>     i = VALUE_B;
> }

You could include this test case in the patch, as an addition to the
test suite.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kamil Dudka Sept. 2, 2009, 4:23 p.m. UTC | #2
On Wednesday 02 of September 2009 17:21:46 Josh Triplett wrote:
> Typo.  I meant ENUM_TYPE_A:
>
>     var_a = (enum ENUM_TYPE_A) 0;
>
> Hopefully that makes more sense as an "always OK" test. :)
>
> Another crazy test for the "always OK" section:
>
>     anon_enum_var = (__typeof__(anon_enum_var)) 0;
>     anon_enum_var = (__typeof__(anon_enum_var)) VALUE_A;

All three cases pass for me, even with -Wenum-to-int. No problem here.

> > -	warn_for_different_enum_types (old->pos, old->ctype, type);
> > +	warn_for_different_enum_types (old->pos, old->ctype, type,
> > +				       old->enum_type);
>
> At this point, it might make more sense to just pass old, rather than
> three of its fields.  Would that work for the other callers of this
> function?  In any case, that change can wait until after this patch.

At first glance the only change would be in usual_conversions() in reporting 
slightly different location in some rear cases -- no big deal I think.

But in fact I haven't investigated yet how to trigger all the particular 
possibilities of warn_for_different_enum_types() invocation to test this 
properly. I'll give it a try later today.

> > +	warn_for_enum_to_int_cast (old, type);
> > +	warn_for_int_to_enum_cast (old, type);
>
> I just realized that both of these functions need renaming as well:
> s/cast/conversion/ or similar.  As with the manpage changes before,
> "cast" describes the case it *doesn't* warn about.

My line of thinking was "implied vs. explicit cast" ... but I am fine with
the rename. It'll be more obvious.

> > --- a/sparse.1
> > +++ b/sparse.1
> > @@ -149,6 +149,19 @@ Sparse issues these warnings by default.  To turn
> > them off, use \fB\-Wno\-enum\-mismatch\fR.
> >  .
> >  .TP
> > +.B \-Wenum\-to\-int
> > +Warn about converting an \fBenum\fR type to int. An explicit cast to int
> > will +suppress this warning.
> > +.
> > +.TP
> > +.B \-Wint\-to\-enum
> > +Warn about converting int to \fBenum\fR type. An explicit cast to the
> > right +\fBenum\fR type will suppress this warning.
> > +
> > +Sparse issues these warnings by default.  To turn them off, use
> > +\fB\-Wno\-int\-to\-enum\fR.
>
> This manpage text looks good to me.

Thanks for review!

> You could include this test case in the patch, as an addition to the
> test suite.

No problem I think. We have generally two choices:

    1) The whole current test-enum.c as a test-case running with all three
       warnings enabled.

    2) Split the test to three parts, each for one separate -W option, and
       then run it as three separate test-cases.

I think the second choice has better coverage. What's your opinion?

Kamil
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li Sept. 2, 2009, 4:38 p.m. UTC | #3
On Wed, Sep 2, 2009 at 9:23 AM, Kamil Dudka<kdudka@redhat.com> wrote:
>
> No problem I think. We have generally two choices:
>
>    1) The whole current test-enum.c as a test-case running with all three
>       warnings enabled.
>
>    2) Split the test to three parts, each for one separate -W option, and
>       then run it as three separate test-cases.

I like option 2) better. Having three test file.

I am playing with your patch right now. I will send out some feed back
soon. Over all I like the patch.

Thanks

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josh Triplett Sept. 2, 2009, 7:03 p.m. UTC | #4
On Wed, Sep 02, 2009 at 06:23:36PM +0200, Kamil Dudka wrote:
> On Wednesday 02 of September 2009 17:21:46 Josh Triplett wrote:
> > Typo.  I meant ENUM_TYPE_A:
> >
> >     var_a = (enum ENUM_TYPE_A) 0;
> >
> > Hopefully that makes more sense as an "always OK" test. :)
> >
> > Another crazy test for the "always OK" section:
> >
> >     anon_enum_var = (__typeof__(anon_enum_var)) 0;
> >     anon_enum_var = (__typeof__(anon_enum_var)) VALUE_A;
> 
> All three cases pass for me, even with -Wenum-to-int. No problem here.

Great.  I didn't expect any of them to fail with your patch; I primarily
suggested them to cover strange corner cases that might occur in future
regressions.

> > > -	warn_for_different_enum_types (old->pos, old->ctype, type);
> > > +	warn_for_different_enum_types (old->pos, old->ctype, type,
> > > +				       old->enum_type);
> >
> > At this point, it might make more sense to just pass old, rather than
> > three of its fields.  Would that work for the other callers of this
> > function?  In any case, that change can wait until after this patch.
> 
> At first glance the only change would be in usual_conversions() in reporting 
> slightly different location in some rear cases -- no big deal I think.
> 
> But in fact I haven't investigated yet how to trigger all the particular 
> possibilities of warn_for_different_enum_types() invocation to test this 
> properly. I'll give it a try later today.

Don't worry about this change.  I only suggested it as a potential
simplification, but it doesn't need to happen as part of this patch.
I'd rather see the patch get merged in its current form (plus the test
suite additions), rather than poking at simplifications like this that
don't immediately prove trivial.  Those can always happen later. :)

> > > +	warn_for_enum_to_int_cast (old, type);
> > > +	warn_for_int_to_enum_cast (old, type);
> >
> > I just realized that both of these functions need renaming as well:
> > s/cast/conversion/ or similar.  As with the manpage changes before,
> > "cast" describes the case it *doesn't* warn about.
> 
> My line of thinking was "implied vs. explicit cast" ... but I am fine with
> the rename. It'll be more obvious.

To me, "cast" always suggests "explicit cast".

> > You could include this test case in the patch, as an addition to the
> > test suite.
> 
> No problem I think. We have generally two choices:
> 
>     1) The whole current test-enum.c as a test-case running with all three
>        warnings enabled.
> 
>     2) Split the test to three parts, each for one separate -W option, and
>        then run it as three separate test-cases.
> 
> I think the second choice has better coverage. What's your opinion?

Either one seems fine; I don't think splitting the test case helps
coverage, and keeping it together lets you use the same declarations for
the entire test case as you did in the previously attached version.
However, I wonder if it would make sense to have the same test case run
multiple times with different warning options and correspondingly
different output, to make sure the warnings stay associated with the
right flag?  Given the current test framework, that would unfortunately
involve some duplication, but it still seems worth doing.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kamil Dudka Sept. 2, 2009, 7:19 p.m. UTC | #5
On Wednesday 02 of September 2009 21:03:41 Josh Triplett wrote:
> Don't worry about this change.  I only suggested it as a potential
> simplification, but it doesn't need to happen as part of this patch.
> I'd rather see the patch get merged in its current form (plus the test
> suite additions), rather than poking at simplifications like this that
> don't immediately prove trivial.  Those can always happen later. :)

Nope, I think we should fix it right now. And if possible ask the original 
authors for review and/or comment at the patch I am currently preparing.
I considered it pretty broken. Just try this example:

static void f(void) {
    enum ENUM_TYPE_A { VALUE_A } var_a;
    enum ENUM_TYPE_B { VALUE_B } var_b;

    switch (var_a) {
        case VALUE_A:
        case VALUE_B:
        default:
            break;
    }
}

It seems like this was the original intention of the calling the 
warn_for_different_enum_types() from check_case_type(). But it has been 
either not tested, or broken in the meantime.

> Either one seems fine; I don't think splitting the test case helps
> coverage, and keeping it together lets you use the same declarations for
> the entire test case as you did in the previously attached version.

This is not necessarily dedicated to choice 1), unless you are scared from
a construction like this:

    #include "enum-common.c"

> However, I wonder if it would make sense to have the same test case run
> multiple times with different warning options and correspondingly
> different output, to make sure the warnings stay associated with the
> right flag?  Given the current test framework, that would unfortunately
> involve some duplication, but it still seems worth doing.

I think the choice 2) slightly wins (counting me and Chris for now)...

Kamil
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

From 381f635e581c3d346ae47672dc7be1423b681924 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Wed, 2 Sep 2009 13:17:57 +0200
Subject: [PATCH] add warnings enum-to-int and int-to-enum...

... and improve detection of the enum-mismatch warning

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 evaluate.c   |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 expression.h |    1 +
 lib.c        |    4 ++
 lib.h        |    2 +
 parse.c      |    1 +
 sparse.1     |   13 ++++++++
 6 files changed, 106 insertions(+), 13 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 805ae90..dfb7a0d 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -235,27 +235,94 @@  static int is_same_type(struct expression *expr, struct symbol *new)
 }
 
 static void
-warn_for_different_enum_types (struct position pos,
-			       struct symbol *typea,
-			       struct symbol *typeb)
+resolve_sym_node (struct symbol **psym)
 {
+	struct symbol *sym = *psym;
+	if (sym->type == SYM_NODE)
+		*psym = sym->ctype.base_type;
+}
+
+static void
+warn_for_different_enum_types (struct position pos, struct symbol *typea,
+			       struct symbol *typeb, struct symbol *enum_type)
+{
+	enum type ttypea;
 	if (!Wenum_mismatch)
 		return;
-	if (typea->type == SYM_NODE)
-		typea = typea->ctype.base_type;
-	if (typeb->type == SYM_NODE)
-		typeb = typeb->ctype.base_type;
+
+	resolve_sym_node(&typea);
+	resolve_sym_node(&typeb);
 
 	if (typea == typeb)
 		return;
+	if (typeb->type != SYM_ENUM)
+		return;
 
-	if (typea->type == SYM_ENUM && typeb->type == SYM_ENUM) {
+	ttypea = typea->type;
+	if (ttypea == SYM_ENUM || (ttypea == SYM_BASETYPE
+					&& enum_type && enum_type != typeb))
+	{
 		warning(pos, "mixing different enum types");
-		info(pos, "    %s versus", show_typename(typea));
+		info(pos, "    %s versus", show_typename((ttypea == SYM_ENUM)
+					? typea
+					: enum_type));
 		info(pos, "    %s", show_typename(typeb));
 	}
 }
 
+static void
+issue_conversion_warning(struct position pos,
+			 struct symbol *typea,
+			 struct symbol *typeb)
+{
+	warning(pos, "conversion of");
+	info(pos, "    %s to", show_typename(typea));
+	info(pos, "    %s", show_typename(typeb));
+}
+
+static void
+warn_for_enum_to_int_cast (struct expression *expr, struct symbol *typeb)
+{
+	struct position pos = expr->pos;
+	struct symbol *typea = expr->ctype;
+	struct symbol *enum_type = expr->enum_type;
+
+	if (!Wenum_to_int)
+		return;
+
+	resolve_sym_node(&typea);
+	resolve_sym_node(&typeb);
+
+	if (typeb->type != SYM_BASETYPE)
+		return;
+
+	if (typea->type == SYM_ENUM && typea->ident)
+		issue_conversion_warning(pos, typea, typeb);
+
+	if (typea->type == SYM_BASETYPE && enum_type && enum_type->ident)
+		issue_conversion_warning(pos, enum_type, typeb);
+}
+
+static void
+warn_for_int_to_enum_cast (struct expression *expr, struct symbol *typeb)
+{
+	struct position pos = expr->pos;
+	struct symbol *typea = expr->ctype;
+	struct symbol *enum_type = expr->enum_type;
+
+	if (!Wint_to_enum)
+		return;
+
+	resolve_sym_node(&typea);
+	resolve_sym_node(&typeb);
+
+	if (typea->type != SYM_BASETYPE || typeb->type != SYM_ENUM)
+		return;
+
+	if (!enum_type)
+		issue_conversion_warning(pos, typea, typeb);
+}
+
 /*
  * This gets called for implicit casts in assignments and
  * integer promotion. We often want to try to move the
@@ -267,7 +334,10 @@  static struct expression * cast_to(struct expression *old, struct symbol *type)
 {
 	struct expression *expr;
 
-	warn_for_different_enum_types (old->pos, old->ctype, type);
+	warn_for_different_enum_types (old->pos, old->ctype, type,
+				       old->enum_type);
+	warn_for_enum_to_int_cast (old, type);
+	warn_for_int_to_enum_cast (old, type);
 
 	if (old->ctype != &null_ctype && is_same_type(old, type))
 		return old;
@@ -287,7 +357,7 @@  static struct expression * cast_to(struct expression *old, struct symbol *type)
 		break;
 
 	case EXPR_IMPLIED_CAST:
-		warn_for_different_enum_types(old->pos, old->ctype, type);
+		warn_for_different_enum_types(old->pos, old->ctype, type, NULL);
 
 		if (old->ctype->bit_size >= type->bit_size) {
 			struct expression *orig = old->cast_expression;
@@ -498,7 +568,8 @@  static struct symbol *usual_conversions(int op,
 {
 	struct symbol *ctype;
 
-	warn_for_different_enum_types(right->pos, left->ctype, right->ctype);
+	warn_for_different_enum_types(right->pos, left->ctype, right->ctype,
+				      NULL);
 
 	if ((lclass | rclass) & TYPE_RESTRICT)
 		goto Restr;
@@ -3241,7 +3312,8 @@  static void check_case_type(struct expression *switch_expr,
 		goto Bad;
 	if (enumcase) {
 		if (*enumcase)
-			warn_for_different_enum_types(case_expr->pos, case_type, (*enumcase)->ctype);
+			warn_for_different_enum_types(case_expr->pos, case_type,
+						      (*enumcase)->ctype, NULL);
 		else if (is_enum_type(case_type))
 			*enumcase = case_expr;
 	}
diff --git a/expression.h b/expression.h
index 631224f..81f70ad 100644
--- a/expression.h
+++ b/expression.h
@@ -70,6 +70,7 @@  struct expression {
 		struct {
 			unsigned long long value;
 			unsigned taint;
+			struct symbol *enum_type;
 		};
 
 		// EXPR_FVALUE
diff --git a/lib.c b/lib.c
index 600939b..2f78bd5 100644
--- a/lib.c
+++ b/lib.c
@@ -199,6 +199,8 @@  int Wdecl = 1;
 int Wdefault_bitfield_sign = 0;
 int Wdo_while = 0;
 int Wenum_mismatch = 1;
+int Wenum_to_int = 0;
+int Wint_to_enum = 1;
 int Wnon_pointer_null = 1;
 int Wold_initializer = 1;
 int Wone_bit_signed_bitfield = 1;
@@ -380,6 +382,8 @@  static const struct warning {
 	{ "default-bitfield-sign", &Wdefault_bitfield_sign },
 	{ "do-while", &Wdo_while },
 	{ "enum-mismatch", &Wenum_mismatch },
+	{ "enum-to-int", &Wenum_to_int },
+	{ "int-to-enum", &Wint_to_enum },
 	{ "non-pointer-null", &Wnon_pointer_null },
 	{ "old-initializer", &Wold_initializer },
 	{ "one-bit-signed-bitfield", &Wone_bit_signed_bitfield },
diff --git a/lib.h b/lib.h
index b22fa93..962d49d 100644
--- a/lib.h
+++ b/lib.h
@@ -96,6 +96,8 @@  extern int Wdecl;
 extern int Wdefault_bitfield_sign;
 extern int Wdo_while;
 extern int Wenum_mismatch;
+extern int Wenum_to_int;
+extern int Wint_to_enum;
 extern int Wnon_pointer_null;
 extern int Wold_initializer;
 extern int Wone_bit_signed_bitfield;
diff --git a/parse.c b/parse.c
index 5e75242..76d4c58 100644
--- a/parse.c
+++ b/parse.c
@@ -791,6 +791,7 @@  static void cast_enum_list(struct symbol_list *list, struct symbol *base_type)
 		if (expr->type != EXPR_VALUE)
 			continue;
 		ctype = expr->ctype;
+		expr->enum_type = sym->ctype.base_type;
 		if (ctype->bit_size == base_type->bit_size)
 			continue;
 		cast_value(expr, base_type, expr, ctype);
diff --git a/sparse.1 b/sparse.1
index d7fe444..288b3a8 100644
--- a/sparse.1
+++ b/sparse.1
@@ -149,6 +149,19 @@  Sparse issues these warnings by default.  To turn them off, use
 \fB\-Wno\-enum\-mismatch\fR.
 .
 .TP
+.B \-Wenum\-to\-int
+Warn about converting an \fBenum\fR type to int. An explicit cast to int will
+suppress this warning.
+.
+.TP
+.B \-Wint\-to\-enum
+Warn about converting int to \fBenum\fR type. An explicit cast to the right
+\fBenum\fR type will suppress this warning.
+
+Sparse issues these warnings by default.  To turn them off, use
+\fB\-Wno\-int\-to\-enum\fR.
+.
+.TP
 .B \-Wnon\-pointer\-null
 Warn about the use of 0 as a NULL pointer.
 
-- 
1.6.2.5