sparse segv with simple test

sparse segv with simple test

[Stephen Hemminger]

I was checking up on some enum issues and tried running the following:
	sparse -Wenum-mismatch enum.c
It dies here:
gdb) run -Wenum-mismatch e.c
Starting program: /home/shemminger/src/sparse/sparse -Wenum-mismatch enum.c

Program received signal SIGSEGV, Segmentation fault.
0x0805c81d in linearize_store_gen (ep=0xb7e7600c, value=0xb7e4e00c, 
    ad=0xbfd43134) at linearize.h:293
293		return (p && p->type != PSEUDO_VOID && p->type != PSEUDO_VAL);
(gdb) where
#0  0x0805c81d in linearize_store_gen (ep=0xb7e7600c, value=0xb7e4e00c, 
    ad=0xbfd43134) at linearize.h:293
#1  0x080609da in linearize_symbol (sym=0xb7e244cc) at linearize.c:1526
#2  0x080494bd in main (argc=-1209907772, argv=0xb7e245c4) at sparse.c:266
------
#include <stdio.h>

enum x { A, B, C };

static enum x foo(int n) {
	return (n > 0) ? A : B;
}

int main(int ac, char **av) {
	int x = foo(ac);
	enum x y = 99;

	printf("%d %d\n", x, y);
	return 0;
}

Re: sparse segv with simple test

[Kamil Dudka]

On Monday 31 of August 2009 00:32:02 Stephen Hemminger wrote:
> I was checking up on some enum issues and tried running the following:
> 	sparse -Wenum-mismatch enum.c
> It dies here:
> gdb) run -Wenum-mismatch e.c
> Starting program: /home/shemminger/src/sparse/sparse -Wenum-mismatch enum.c
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x0805c81d in linearize_store_gen (ep=0xb7e7600c, value=0xb7e4e00c,
>     ad=0xbfd43134) at linearize.h:293
> 293		return (p && p->type != PSEUDO_VOID && p->type != PSEUDO_VAL);
> (gdb) where
> #0  0x0805c81d in linearize_store_gen (ep=0xb7e7600c, value=0xb7e4e00c,
>     ad=0xbfd43134) at linearize.h:293
> #1  0x080609da in linearize_symbol (sym=0xb7e244cc) at linearize.c:1526
> #2  0x080494bd in main (argc=-1209907772, argv=0xb7e245c4) at sparse.c:266
> ------
> #include <stdio.h>
>
> enum x { A, B, C };
>
> static enum x foo(int n) {
> 	return (n > 0) ? A : B;
> }
>
> int main(int ac, char **av) {
> 	int x = foo(ac);
> 	enum x y = 99;
>
> 	printf("%d %d\n", x, y);
> 	return 0;
> }

I am unable to reproduce the crash, tested with sparse 0.4.1 and current git 
HEAD. Even no suspicious place reported by valgrind. Maybe your <stdio.h> is 
the trigger. Could you please attach the preprocessed code?

Kamil

Re: sparse segv with simple test

[Stephen Hemminger]

On Mon, 31 Aug 2009 00:53:58 +0200
Kamil Dudka <kdudka@redhat.com> wrote:

> On Monday 31 of August 2009 00:32:02 Stephen Hemminger wrote:
> > I was checking up on some enum issues and tried running the following:
> > 	sparse -Wenum-mismatch enum.c
> > It dies here:
> > gdb) run -Wenum-mismatch e.c
> > Starting program: /home/shemminger/src/sparse/sparse -Wenum-mismatch enum.c
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x0805c81d in linearize_store_gen (ep=0xb7e7600c, value=0xb7e4e00c,
> >     ad=0xbfd43134) at linearize.h:293
> > 293		return (p && p->type != PSEUDO_VOID && p->type != PSEUDO_VAL);
> > (gdb) where
> > #0  0x0805c81d in linearize_store_gen (ep=0xb7e7600c, value=0xb7e4e00c,
> >     ad=0xbfd43134) at linearize.h:293
> > #1  0x080609da in linearize_symbol (sym=0xb7e244cc) at linearize.c:1526
> > #2  0x080494bd in main (argc=-1209907772, argv=0xb7e245c4) at sparse.c:266
> > ------
> > #include <stdio.h>
> >
> > enum x { A, B, C };
> >
> > static enum x foo(int n) {
> > 	return (n > 0) ? A : B;
> > }
> >
> > int main(int ac, char **av) {
> > 	int x = foo(ac);
> > 	enum x y = 99;
> >
> > 	printf("%d %d\n", x, y);
> > 	return 0;
> > }
> 
> I am unable to reproduce the crash, tested with sparse 0.4.1 and current git 
> HEAD. Even no suspicious place reported by valgrind. Maybe your <stdio.h> is 
> the trigger. Could you please attach the preprocessed code?
> 
> Kamil

Not sure why, but the crash went away after a 'make clean; make' so maybe
something was stale on that machine?

It still doesn't give any warnings. about assigning an enum with a value
out of range thou.

-- 

Re: sparse segv with simple test

[Kamil Dudka]

On Monday 31 of August 2009 17:57:58 Stephen Hemminger wrote:
> Not sure why, but the crash went away after a 'make clean; make' so maybe
> something was stale on that machine?

Could be ... outdated depfiles, skewed mtimes, etc.

> It still doesn't give any warnings. about assigning an enum with a value
> out of range thou.

AFAICT it is not supposed to issue a warning in that case. Here is an example 
which is caught by the check:

static void f(void) {
    enum A { VAL_A } a = VAL_A;
    enum B { VAL_B } b = a;
}

$ sparse enum.c
enum.c:3:26: warning: mixing different enum types
enum.c:3:26:     int enum A  versus
enum.c:3:26:     int enum B

Note that you don't even need to write -Wenum-mismatch since it's default.

But it might be good idea to implement such out-of-range detection ;-)

Kamil

Re: sparse segv with simple test

[Stephen Hemminger]

On Mon, 31 Aug 2009 20:12:36 +0200
Kamil Dudka <kdudka@redhat.com> wrote:

> On Monday 31 of August 2009 17:57:58 Stephen Hemminger wrote:
> > Not sure why, but the crash went away after a 'make clean; make' so maybe
> > something was stale on that machine?
> 
> Could be ... outdated depfiles, skewed mtimes, etc.
> 
> > It still doesn't give any warnings. about assigning an enum with a value
> > out of range thou.
> 
> AFAICT it is not supposed to issue a warning in that case. Here is an example 
> which is caught by the check:
> 
> static void f(void) {
>     enum A { VAL_A } a = VAL_A;
>     enum B { VAL_B } b = a;
> }
> 
> $ sparse enum.c
> enum.c:3:26: warning: mixing different enum types
> enum.c:3:26:     int enum A  versus
> enum.c:3:26:     int enum B
> 
> Note that you don't even need to write -Wenum-mismatch since it's default.
> 
> But it might be good idea to implement such out-of-range detection ;-)

Yes, I was looking to use more enum's in places where int was used in the
kernel. There were places where the interface expected a limit range of
values like network device transmit, but the interface was being misused
and caller was returning errno values.

Re: sparse segv with simple test

[Kamil Dudka]

On Monday 31 of August 2009 20:49:00 Stephen Hemminger wrote:
> > But it might be good idea to implement such out-of-range detection ;-)
>
> Yes, I was looking to use more enum's in places where int was used in the
> kernel. There were places where the interface expected a limit range of
> values like network device transmit, but the interface was being misused
> and caller was returning errno values.

Generally you can't simply check if the value is in a *range* since the enum 
values could be non-continuous. You need to check for all continuous 
sub-ranges or even for all values.

What about issuing a warning for any assignment of int value to enum? (as it 
is already checked by a C++ compiler) Such places are IMHO always suspicious. 
If you know what you are doing you can still cast it explicitly to avoid such 
warning.

Kamil

Re: sparse segv with simple test

[Josh Triplett]

On Mon, Aug 31, 2009 at 09:04:54PM +0200, Kamil Dudka wrote:
> On Monday 31 of August 2009 20:49:00 Stephen Hemminger wrote:
> > > But it might be good idea to implement such out-of-range detection ;-)
> >
> > Yes, I was looking to use more enum's in places where int was used in the
> > kernel. There were places where the interface expected a limit range of
> > values like network device transmit, but the interface was being misused
> > and caller was returning errno values.
> 
> Generally you can't simply check if the value is in a *range* since the enum 
> values could be non-continuous. You need to check for all continuous 
> sub-ranges or even for all values.
> 
> What about issuing a warning for any assignment of int value to enum? (as it 
> is already checked by a C++ compiler) Such places are IMHO always suspicious. 
> If you know what you are doing you can still cast it explicitly to avoid such 
> warning.

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;".)

- Josh Triplett

[PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

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

Kamil

[-- Attachment #2: test-enum.c --]
[-- Type: text/x-csrc, Size: 755 bytes --]

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;
    i = VALUE_B;
    anon_enum_var = VALUE_C;
    i = VALUE_C;
    i = anon_enum_var;

    // 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;
    var_a = 0;
    var_b = i;
    anon_enum_var = 0;
    anon_enum_var = i;

    // caught only with -Wenum-to-int
    i = var_a;
}

[-- Attachment #3: 0001-add-warnings-enum-to-int-and-int-to-enum.patch --]
[-- Type: text/x-diff, Size: 5498 bytes --]

From a5095bbdb9694effa4a771295a87b80bf84d3230 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Tue, 1 Sep 2009 23:51:29 +0200
Subject: [PATCH] add warnings enum-to-int and int-to-enum


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

diff --git a/evaluate.c b/evaluate.c
index 805ae90..47e50fb 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -235,16 +235,23 @@ static int is_same_type(struct expression *expr, struct symbol *new)
 }
 
 static void
+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)
 {
 	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;
@@ -256,6 +263,54 @@ warn_for_different_enum_types (struct position pos,
 	}
 }
 
+static void
+warn_for_enum_to_int_cast (struct expression *expr, struct symbol *typeb)
+{
+	struct position pos = expr->pos;
+	struct symbol *typea = expr->ctype;
+
+	if (!Wenum_to_int)
+		return;
+
+	resolve_sym_node(&typea);
+	resolve_sym_node(&typeb);
+
+	if (typea->type != SYM_ENUM || typeb->type != SYM_BASETYPE)
+		return;
+
+	if (typea->ident) {
+		warning(pos, "cast from");
+		info(pos, "    %s to", show_typename(typea));
+		info(pos, "    %s", show_typename(typeb));
+		return;
+	}
+}
+
+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 || (enum_type != typeb)) {
+		warning(pos, "cast from");
+		info(pos, "    %s to", show_typename((enum_type)
+					? enum_type
+					: typea));
+		info(pos, "    %s", show_typename(typeb));
+	}
+}
+
 /*
  * This gets called for implicit casts in assignments and
  * integer promotion. We often want to try to move the
@@ -268,6 +323,8 @@ 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_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;
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..7eb0cda 100644
--- 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.
+.
+.TP
+.B \-Wint\-to\-enum
+Warn about casting of int (or incompatible enumeration constant) to \fBenum\fR.
+
+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.4.1

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Josh Triplett]

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

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Stephen Hemminger]

On Tue, 1 Sep 2009 16:24:33 -0700
Josh Triplett <josh@joshtriplett.org> 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.





-- 

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

[-- Attachment #1: Type: text/plain, Size: 4307 bytes --]

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

[-- Attachment #2: 0001-add-warnings-enum-to-int-and-int-to-enum.patch --]
[-- Type: text/x-patch, Size: 7546 bytes --]

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


[-- Attachment #3: test-enum.c --]
[-- Type: text/x-csrc, Size: 941 bytes --]

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;
}

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Josh Triplett]

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

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

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

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Christopher Li]

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

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Daniel Barkalow]

On Tue, 1 Sep 2009, Stephen Hemminger wrote:

> there is lots of code that does:
> 
> enum {
>    my_register_1   = 0x001,
>    my_register_2   = 0x002,
> };

It feels to me like the explicit numeric values are what make these 
constants sensible to use directly as ints, and that it's only sensible to 
use a non-constant value of an enum type as an int (without an explicit 
cast) if all of the enum values have explicit numeric values.

I think:

  enum {
    my_register_zero
    ...
    my_register_twdr
    my_register_twcr
    ...
  };

  void () {
    write_register(my_register_twdr, SETUP_TWDR);
  }

is asking for trouble in a way that this warning is about.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

On Wednesday 02 of September 2009 19:56:47 Daniel Barkalow wrote:
> It feels to me like the explicit numeric values are what make these
> constants sensible to use directly as ints, and that it's only sensible to
> use a non-constant value of an enum type as an int (without an explicit
> cast) if all of the enum values have explicit numeric values.
>
> I think:
>
>   enum {
>     my_register_zero
>     ...
>     my_register_twdr
>     my_register_twcr
>     ...
>   };
>
>   void () {
>     write_register(my_register_twdr, SETUP_TWDR);
>   }
>
> is asking for trouble in a way that this warning is about.

Both examples are too abstract for me -- missing declaration
of write_register(), etc. Please attach a minimal example as a file which I 
can compile and test. I'll check if the "trouble" is covered by the warnings 
or not, and perhaps implement what's missing. Thanks in advance!

Kamil

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Daniel Barkalow]

On Wed, 2 Sep 2009, Kamil Dudka wrote:

> On Wednesday 02 of September 2009 19:56:47 Daniel Barkalow wrote:
> > It feels to me like the explicit numeric values are what make these
> > constants sensible to use directly as ints, and that it's only sensible to
> > use a non-constant value of an enum type as an int (without an explicit
> > cast) if all of the enum values have explicit numeric values.
> >
> > I think:
> >
> >   enum {
> >     my_register_zero
> >     ...
> >     my_register_twdr
> >     my_register_twcr
> >     ...
> >   };
> >
> >   void () {
> >     write_register(my_register_twdr, SETUP_TWDR);
> >   }
> >
> > is asking for trouble in a way that this warning is about.
> 
> Both examples are too abstract for me -- missing declaration
> of write_register(), etc. Please attach a minimal example as a file which I 
> can compile and test. I'll check if the "trouble" is covered by the warnings 
> or not, and perhaps implement what's missing. Thanks in advance!

enum {
  foo,
  bar
};

enum {
  baz = 1,
  qux = 2
};

void test(void) {
  int i = bar; // warn on this
  int j = qux; // okay
}

(Leaving aside the issue of whether the enum is anonymous)

In the "bar" case, an additional value added somewhere in the list 
(particularly if the list were long) might change "i" in a way that 
wouldn't necessarily be obvious to users of "i". In the "qux" case, "j" 
would only change if the "qux = 2" line were changed.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Josh Triplett]

On Wed, Sep 02, 2009 at 02:43:52PM -0400, Daniel Barkalow wrote:
> On Wed, 2 Sep 2009, Kamil Dudka wrote:
> 
> > On Wednesday 02 of September 2009 19:56:47 Daniel Barkalow wrote:
> > > It feels to me like the explicit numeric values are what make these
> > > constants sensible to use directly as ints, and that it's only sensible to
> > > use a non-constant value of an enum type as an int (without an explicit
> > > cast) if all of the enum values have explicit numeric values.
> > >
> > > I think:
> > >
> > >   enum {
> > >     my_register_zero
> > >     ...
> > >     my_register_twdr
> > >     my_register_twcr
> > >     ...
> > >   };
> > >
> > >   void () {
> > >     write_register(my_register_twdr, SETUP_TWDR);
> > >   }
> > >
> > > is asking for trouble in a way that this warning is about.
> > 
> > Both examples are too abstract for me -- missing declaration
> > of write_register(), etc. Please attach a minimal example as a file which I 
> > can compile and test. I'll check if the "trouble" is covered by the warnings 
> > or not, and perhaps implement what's missing. Thanks in advance!
> 
> enum {
>   foo,
>   bar
> };
> 
> enum {
>   baz = 1,
>   qux = 2
> };
> 
> void test(void) {
>   int i = bar; // warn on this
>   int j = qux; // okay
> }
> 
> (Leaving aside the issue of whether the enum is anonymous)
> 
> In the "bar" case, an additional value added somewhere in the list 
> (particularly if the list were long) might change "i" in a way that 
> wouldn't necessarily be obvious to users of "i". In the "qux" case, "j" 
> would only change if the "qux = 2" line were changed.

I disagree with this.  In both cases, you've declared an anonymous enum,
and then used its values as constants.  In the former case, you might
just not care about the values except to compare against each other;
granted, you ought to use a named enum rather than an int in that case,
but nevertheless much code exists that uses int instead of enum types.

If sparse warned about using the values from an anonymous enum if not
declared with an explicit value, it might as well just warn about the
declaration of the enum in the first place, since you can't do anything
useful with it other than using its values as constants.  That seems
like a warning Sparse could have, with a separate warning flag
(-Wanonymous-enum-implicit-value or similar); however, I don't think
that warning should appear by default, nor should it appear as part of
one of the existing flags under discussion.  I don't think it seems
very sensible to have at all, really, but *shrug*.

- Josh Triplett

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Josh Triplett]

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

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Daniel Barkalow]

On Wed, 2 Sep 2009, Josh Triplett wrote:

> On Wed, Sep 02, 2009 at 02:43:52PM -0400, Daniel Barkalow wrote:
> > On Wed, 2 Sep 2009, Kamil Dudka wrote:
> > 
> > > On Wednesday 02 of September 2009 19:56:47 Daniel Barkalow wrote:
> > > > It feels to me like the explicit numeric values are what make these
> > > > constants sensible to use directly as ints, and that it's only sensible to
> > > > use a non-constant value of an enum type as an int (without an explicit
> > > > cast) if all of the enum values have explicit numeric values.
> > > >
> > > > I think:
> > > >
> > > >   enum {
> > > >     my_register_zero
> > > >     ...
> > > >     my_register_twdr
> > > >     my_register_twcr
> > > >     ...
> > > >   };
> > > >
> > > >   void () {
> > > >     write_register(my_register_twdr, SETUP_TWDR);
> > > >   }
> > > >
> > > > is asking for trouble in a way that this warning is about.
> > > 
> > > Both examples are too abstract for me -- missing declaration
> > > of write_register(), etc. Please attach a minimal example as a file which I 
> > > can compile and test. I'll check if the "trouble" is covered by the warnings 
> > > or not, and perhaps implement what's missing. Thanks in advance!
> > 
> > enum {
> >   foo,
> >   bar
> > };
> > 
> > enum {
> >   baz = 1,
> >   qux = 2
> > };
> > 
> > void test(void) {
> >   int i = bar; // warn on this
> >   int j = qux; // okay
> > }
> > 
> > (Leaving aside the issue of whether the enum is anonymous)
> > 
> > In the "bar" case, an additional value added somewhere in the list 
> > (particularly if the list were long) might change "i" in a way that 
> > wouldn't necessarily be obvious to users of "i". In the "qux" case, "j" 
> > would only change if the "qux = 2" line were changed.
> 
> I disagree with this.  In both cases, you've declared an anonymous enum,
> and then used its values as constants.  In the former case, you might
> just not care about the values except to compare against each other;
> granted, you ought to use a named enum rather than an int in that case,
> but nevertheless much code exists that uses int instead of enum types.

I think anonymous vs named enums are one thing that should affect whether 
you get a warning, and explicit values vs implicit values are another 
factor, but I don't have an opinion on which way they should combine. 
Probably either using an explicit value or an anonymous enum should be 
okay by default, and the test above should use named enums.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

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

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

On Wednesday 02 of September 2009 21:19:46 Daniel Barkalow wrote:
> On Wed, 2 Sep 2009, Josh Triplett wrote:
> > On Wed, Sep 02, 2009 at 02:43:52PM -0400, Daniel Barkalow wrote:
> > > On Wed, 2 Sep 2009, Kamil Dudka wrote:
> > > > On Wednesday 02 of September 2009 19:56:47 Daniel Barkalow wrote:
> > > > > It feels to me like the explicit numeric values are what make these
> > > > > constants sensible to use directly as ints, and that it's only
> > > > > sensible to use a non-constant value of an enum type as an int
> > > > > (without an explicit cast) if all of the enum values have explicit
> > > > > numeric values.
> > > > >
> > > > > I think:
> > > > >
> > > > >   enum {
> > > > >     my_register_zero
> > > > >     ...
> > > > >     my_register_twdr
> > > > >     my_register_twcr
> > > > >     ...
> > > > >   };
> > > > >
> > > > >   void () {
> > > > >     write_register(my_register_twdr, SETUP_TWDR);
> > > > >   }
> > > > >
> > > > > is asking for trouble in a way that this warning is about.
> > > >
> > > > Both examples are too abstract for me -- missing declaration
> > > > of write_register(), etc. Please attach a minimal example as a file
> > > > which I can compile and test. I'll check if the "trouble" is covered
> > > > by the warnings or not, and perhaps implement what's missing. Thanks
> > > > in advance!
> > >
> > > enum {
> > >   foo,
> > >   bar
> > > };
> > >
> > > enum {
> > >   baz = 1,
> > >   qux = 2
> > > };
> > >
> > > void test(void) {
> > >   int i = bar; // warn on this
> > >   int j = qux; // okay
> > > }
> > >
> > > (Leaving aside the issue of whether the enum is anonymous)
> > >
> > > In the "bar" case, an additional value added somewhere in the list
> > > (particularly if the list were long) might change "i" in a way that
> > > wouldn't necessarily be obvious to users of "i". In the "qux" case, "j"
> > > would only change if the "qux = 2" line were changed.
> >
> > I disagree with this.  In both cases, you've declared an anonymous enum,
> > and then used its values as constants.  In the former case, you might
> > just not care about the values except to compare against each other;
> > granted, you ought to use a named enum rather than an int in that case,
> > but nevertheless much code exists that uses int instead of enum types.
>
> I think anonymous vs named enums are one thing that should affect whether
> you get a warning, and explicit values vs implicit values are another
> factor, but I don't have an opinion on which way they should combine.
> Probably either using an explicit value or an anonymous enum should be
> okay by default, and the test above should use named enums.

I think type-checking is the way to go, not values checking... Once you start 
to count with "explicit" vs. "implicit" enum values, you will sooner or later 
run in troubles like this:

enum { A, B = 3, C, D = A + 7 };
enum { Z = (C < D) ? A : B };

Kamil

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

On Wednesday 02 of September 2009 21:19:49 Kamil Dudka wrote:
> 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.

The second holds. It's regression!

It used to work in ca1f2e9ebf33e78c77c79eb695fe88d843b4f978 - see the log 
message for a test case. The same test case is failing to issue a warning 
now, but it works again with my patch applied.

Al, Morten, can I ask you as original authors for approval of the patch?

Details about warnings (what? when? why?) are in the tests themselves. Don't 
hesitate to ask me if something is missing there :-)

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

Nobody has objected, so here it is. I think it'll be much easier to maintain 
this way...

Kamil

[-- Attachment #2: 0001-add-warnings-enum-to-int-and-int-to-enum.patch --]
[-- Type: text/x-diff, Size: 17297 bytes --]

From 050663577b8832ee40f4f365bee61e2ed1bf9613 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Wed, 2 Sep 2009 23:49:09 +0200
Subject: [PATCH] add warnings enum-to-int and int-to-enum...

... and improve detection of the enum-mismatch warning. Add test case
for each of the warnings. For details about what triggers which warning
just look to the corresponding test case.

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 evaluate.c                 |  122 ++++++++++++++++++++++++++++++++++----------
 expression.h               |    1 +
 lib.c                      |    4 ++
 lib.h                      |    2 +
 parse.c                    |    1 +
 sparse.1                   |   13 +++++
 validation/enum-common.c   |  112 ++++++++++++++++++++++++++++++++++++++++
 validation/enum-from-int.c |   36 +++++++++++++
 validation/enum-mismatch.c |   48 +++++++++++++++++
 validation/enum-to-int.c   |   27 ++++++++++
 10 files changed, 339 insertions(+), 27 deletions(-)
 create mode 100644 validation/enum-common.c
 create mode 100644 validation/enum-from-int.c
 create mode 100644 validation/enum-mismatch.c
 create mode 100644 validation/enum-to-int.c

diff --git a/evaluate.c b/evaluate.c
index 805ae90..0d5e93b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -235,27 +235,105 @@ 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 expression *expr, struct symbol *typeb)
+{
+	struct position pos = expr->pos;
+	struct symbol *typea = expr->ctype;
+	struct symbol *enum_type = expr->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_conversion (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_conversion (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);
+}
+
+static void
+warn_for_enum_conversions(struct expression *expr, struct symbol *type)
+{
+	warn_for_different_enum_types (expr, type);
+	warn_for_enum_to_int_conversion (expr, type);
+	warn_for_int_to_enum_conversion (expr, type);
+}
+
 /*
  * This gets called for implicit casts in assignments and
  * integer promotion. We often want to try to move the
@@ -267,7 +345,7 @@ 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_enum_conversions(old, type);
 
 	if (old->ctype != &null_ctype && is_same_type(old, type))
 		return old;
@@ -287,8 +365,6 @@ 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);
-
 		if (old->ctype->bit_size >= type->bit_size) {
 			struct expression *orig = old->cast_expression;
 			if (same_cast_type(orig->ctype, type))
@@ -498,7 +574,8 @@ static struct symbol *usual_conversions(int op,
 {
 	struct symbol *ctype;
 
-	warn_for_different_enum_types(right->pos, left->ctype, right->ctype);
+	/* FIXME: we should either write a test-case for it or delete it */
+	warn_for_enum_conversions(left, right->ctype);
 
 	if ((lclass | rclass) & TYPE_RESTRICT)
 		goto Restr;
@@ -3225,8 +3302,7 @@ static void evaluate_case_statement(struct statement *stmt)
 }
 
 static void check_case_type(struct expression *switch_expr,
-			    struct expression *case_expr,
-			    struct expression **enumcase)
+			    struct expression *case_expr)
 {
 	struct symbol *switch_type, *case_type;
 	int sclass, cclass;
@@ -3239,12 +3315,8 @@ static void check_case_type(struct expression *switch_expr,
 
 	if (!switch_type || !case_type)
 		goto Bad;
-	if (enumcase) {
-		if (*enumcase)
-			warn_for_different_enum_types(case_expr->pos, case_type, (*enumcase)->ctype);
-		else if (is_enum_type(case_type))
-			*enumcase = case_expr;
-	}
+
+	warn_for_enum_conversions(case_expr, switch_type);
 
 	sclass = classify_type(switch_type, &switch_type);
 	cclass = classify_type(case_type, &case_type);
@@ -3275,21 +3347,17 @@ Bad:
 static void evaluate_switch_statement(struct statement *stmt)
 {
 	struct symbol *sym;
-	struct expression *enumcase = NULL;
-	struct expression **enumcase_holder = &enumcase;
 	struct expression *sel = stmt->switch_expression;
 
 	evaluate_expression(sel);
 	evaluate_statement(stmt->switch_statement);
 	if (!sel)
 		return;
-	if (sel->ctype && is_enum_type(sel->ctype))
-		enumcase_holder = NULL; /* Only check cases against switch */
 
 	FOR_EACH_PTR(stmt->switch_case->symbol_list, sym) {
 		struct statement *case_stmt = sym->stmt;
-		check_case_type(sel, case_stmt->case_expression, enumcase_holder);
-		check_case_type(sel, case_stmt->case_to, enumcase_holder);
+		check_case_type(sel, case_stmt->case_expression);
+		check_case_type(sel, case_stmt->case_to);
 	} END_FOR_EACH_PTR(sym);
 }
 
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.
 
diff --git a/validation/enum-common.c b/validation/enum-common.c
new file mode 100644
index 0000000..f940fef
--- /dev/null
+++ b/validation/enum-common.c
@@ -0,0 +1,112 @@
+static enum ENUM_TYPE_A { VALUE_A } var_a;
+static enum ENUM_TYPE_B { VALUE_B } var_b;
+static enum /* anon. */ { VALUE_C } anon_enum_var;
+static int i;
+
+static void take_enum_of_type_a(enum ENUM_TYPE_A arg_enum)
+{
+	(void) arg_enum;
+}
+
+static void take_int(int arg_int)
+{
+	(void) arg_int;
+}
+
+static void always_ok(void)
+{
+	var_a ++;
+	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;
+	var_a = (enum ENUM_TYPE_A) 0;
+	anon_enum_var = (__typeof__(anon_enum_var)) 0;
+	anon_enum_var = (__typeof__(anon_enum_var)) VALUE_A;
+
+	switch (var_a) {
+		case VALUE_A:
+		default:
+			take_enum_of_type_a(var_a);
+			take_enum_of_type_a(VALUE_A);
+	}
+
+	switch (anon_enum_var) {
+		case VALUE_C:
+		default:
+			take_int(anon_enum_var);
+	}
+
+	switch (i) {
+		case VALUE_C:
+		default:
+			take_int(VALUE_C);
+	}
+}
+
+static void trigger_enum_mismatch(void)
+{
+	switch (var_a) {
+		case VALUE_B:
+		case VALUE_C:
+		default:
+			take_enum_of_type_a(var_b);
+			take_enum_of_type_a(VALUE_B);
+	}
+
+	switch (anon_enum_var) {
+		case VALUE_A:
+		default:
+			take_enum_of_type_a(anon_enum_var);
+			take_enum_of_type_a(VALUE_C);
+	}
+
+	// this has been already working in sparse 0.4.1
+	var_a = var_b;
+	var_b = anon_enum_var;
+	anon_enum_var = var_a;
+
+	// implemented after sparse 0.4.1
+	var_a = VALUE_B;
+	var_b = VALUE_C;
+	anon_enum_var = VALUE_A;
+}
+
+static void trigger_int_to_enum_conversion(void)
+{
+	switch (var_a) {
+		case 0:
+		default:
+			take_enum_of_type_a(i);
+			take_enum_of_type_a(7);
+	}
+	var_a = 0;
+	var_b = i;
+	anon_enum_var = 0;
+	anon_enum_var = i;
+	var_a = (int) VALUE_A;
+	var_a = (int) VALUE_B;
+}
+
+static void trigger_enum_to_int_conversion(void)
+{
+	i = var_a;
+	i = VALUE_B;
+	switch (i) {
+		case VALUE_A:
+		case VALUE_B:
+		default:
+			take_int(var_a);
+			take_int(VALUE_B);
+	}
+}
+
+/*
+ * check-name: enum-common
+ * check-description: common part of the test for -Wenum-mismatch, -Wenum-to-int and -Wint-to-enum
+ * check-command: sparse -Wno-enum-mismatch -Wno-int-to-enum $file
+ */
diff --git a/validation/enum-from-int.c b/validation/enum-from-int.c
new file mode 100644
index 0000000..15b1e4d
--- /dev/null
+++ b/validation/enum-from-int.c
@@ -0,0 +1,36 @@
+#include "enum-common.c"
+
+/*
+ * check-name: -Wint-to-enum
+ * check-command: sparse -Wno-enum-mismatch $file
+ *
+ * check-error-start
+enum-common.c:84:45: warning: conversion of
+enum-common.c:84:45:     int to
+enum-common.c:84:45:     int enum ENUM_TYPE_A 
+enum-common.c:85:45: warning: conversion of
+enum-common.c:85:45:     int to
+enum-common.c:85:45:     int enum ENUM_TYPE_A 
+enum-common.c:82:22: warning: conversion of
+enum-common.c:82:22:     int to
+enum-common.c:82:22:     int enum ENUM_TYPE_A 
+enum-common.c:87:17: warning: conversion of
+enum-common.c:87:17:     int to
+enum-common.c:87:17:     int enum ENUM_TYPE_A 
+enum-common.c:88:17: warning: conversion of
+enum-common.c:88:17:     int to
+enum-common.c:88:17:     int enum ENUM_TYPE_B 
+enum-common.c:89:25: warning: conversion of
+enum-common.c:89:25:     int to
+enum-common.c:89:25:     int enum <noident> 
+enum-common.c:90:25: warning: conversion of
+enum-common.c:90:25:     int to
+enum-common.c:90:25:     int enum <noident> 
+enum-common.c:91:18: warning: conversion of
+enum-common.c:91:18:     int to
+enum-common.c:91:18:     int enum ENUM_TYPE_A 
+enum-common.c:92:18: warning: conversion of
+enum-common.c:92:18:     int to
+enum-common.c:92:18:     int enum ENUM_TYPE_A 
+ * check-error-end
+ */
diff --git a/validation/enum-mismatch.c b/validation/enum-mismatch.c
new file mode 100644
index 0000000..6db016f
--- /dev/null
+++ b/validation/enum-mismatch.c
@@ -0,0 +1,48 @@
+#include "enum-common.c"
+
+/*
+ * check-name: -Wenum-mismatch
+ * check-command: sparse -Wenum-mismatch -Wno-int-to-enum $file
+ *
+ * check-error-start
+enum-common.c:57:45: warning: mixing different enum types
+enum-common.c:57:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:57:45:     int enum ENUM_TYPE_A 
+enum-common.c:58:45: warning: mixing different enum types
+enum-common.c:58:45:     int enum ENUM_TYPE_B  versus
+enum-common.c:58:45:     int enum ENUM_TYPE_A 
+enum-common.c:54:22: warning: mixing different enum types
+enum-common.c:54:22:     int enum ENUM_TYPE_B  versus
+enum-common.c:54:22:     int enum ENUM_TYPE_A 
+enum-common.c:55:22: warning: mixing different enum types
+enum-common.c:55:22:     int enum <noident>  versus
+enum-common.c:55:22:     int enum ENUM_TYPE_A 
+enum-common.c:64:45: warning: mixing different enum types
+enum-common.c:64:45:     int enum <noident>  versus
+enum-common.c:64:45:     int enum ENUM_TYPE_A 
+enum-common.c:65:45: warning: mixing different enum types
+enum-common.c:65:45:     int enum <noident>  versus
+enum-common.c:65:45:     int enum ENUM_TYPE_A 
+enum-common.c:62:22: warning: mixing different enum types
+enum-common.c:62:22:     int enum ENUM_TYPE_A  versus
+enum-common.c:62:22:     int enum <noident> 
+enum-common.c:69:17: warning: mixing different enum types
+enum-common.c:69:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:69:17:     int enum ENUM_TYPE_A 
+enum-common.c:70:17: warning: mixing different enum types
+enum-common.c:70:17:     int enum <noident>  versus
+enum-common.c:70:17:     int enum ENUM_TYPE_B 
+enum-common.c:71:25: warning: mixing different enum types
+enum-common.c:71:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:71:25:     int enum <noident> 
+enum-common.c:74:17: warning: mixing different enum types
+enum-common.c:74:17:     int enum ENUM_TYPE_B  versus
+enum-common.c:74:17:     int enum ENUM_TYPE_A 
+enum-common.c:75:17: warning: mixing different enum types
+enum-common.c:75:17:     int enum <noident>  versus
+enum-common.c:75:17:     int enum ENUM_TYPE_B 
+enum-common.c:76:25: warning: mixing different enum types
+enum-common.c:76:25:     int enum ENUM_TYPE_A  versus
+enum-common.c:76:25:     int enum <noident> 
+ * check-error-end
+ */
diff --git a/validation/enum-to-int.c b/validation/enum-to-int.c
new file mode 100644
index 0000000..a981ce5
--- /dev/null
+++ b/validation/enum-to-int.c
@@ -0,0 +1,27 @@
+#include "enum-common.c"
+
+/*
+ * check-name: -Wenum-to-int
+ * check-command: sparse -Wenum-to-int -Wno-enum-mismatch -Wno-int-to-enum $file
+ *
+ * check-error-start
+enum-common.c:97:13: warning: conversion of
+enum-common.c:97:13:     int enum ENUM_TYPE_A  to
+enum-common.c:97:13:     int
+enum-common.c:98:13: warning: conversion of
+enum-common.c:98:13:     int enum ENUM_TYPE_B  to
+enum-common.c:98:13:     int
+enum-common.c:103:34: warning: conversion of
+enum-common.c:103:34:     int enum ENUM_TYPE_A  to
+enum-common.c:103:34:     int
+enum-common.c:104:34: warning: conversion of
+enum-common.c:104:34:     int enum ENUM_TYPE_B  to
+enum-common.c:104:34:     int
+enum-common.c:100:22: warning: conversion of
+enum-common.c:100:22:     int enum ENUM_TYPE_A  to
+enum-common.c:100:22:     int
+enum-common.c:101:22: warning: conversion of
+enum-common.c:101:22:     int enum ENUM_TYPE_B  to
+enum-common.c:101:22:     int
+ * check-error-end
+ */
-- 
1.6.4.1

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Christopher Li]

On Wed, Sep 2, 2009 at 3:35 PM, Kamil Dudka<kdudka@redhat.com> wrote:
> On Wednesday 02 of September 2009 21:19:49 Kamil Dudka wrote:
>
> The second holds. It's regression!

I have some questions regarding your patch:

Can we just set the expression->ctype to the enum type
instead of adding the *enum_type? I think the current expr->ctype
can be reached from enum_type->ctype.base_type any way.
In other words, we do care about expression is enum type vs int type
in this patch.

After the type evaluation(and possible warning), we can convert that
enum type back to the base int type because the back end does not
care about enum.

I think fixing the regression should be a separate patch from
the this patch which adding new feature. It is easier to review
as well.

Sorry the rest of the patch will take me more time to go over.

Chris

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

On Thu September 3 2009 11:42:30 Christopher Li wrote:
> Can we just set the expression->ctype to the enum type
> instead of adding the *enum_type? I think the current expr->ctype
> can be reached from enum_type->ctype.base_type any way.
> In other words, we do care about expression is enum type vs int type
> in this patch.
>
> After the type evaluation(and possible warning), we can convert that
> enum type back to the base int type because the back end does not
> care about enum.

Do you want to store the correct enum type to expression->ctype and overwrite 
it later with some integral type? What's the advantage of this approach? 
(instead of preserving ABI which is already changed to 0.4.1). I don't think 
the information about type for enum constant is useful only for issuing 
warnings like this. Some SPARSE clients might also need such information.

> I think fixing the regression should be a separate patch from
> the this patch which adding new feature. It is easier to review
> as well.

Sure if anybody is able to split it... Unfortunately we have no reference for 
the "original" behavior of -Wmismatch-enum. I didn't found it documented 
anywhere and the "test-case" from the referred commit is not really useful as 
a test-case. I have not enough time for reverse engineering...

I think the tests included in the patch have good enough coverage to prevent 
you from a future regression and it sounds more important to me than 
documenting previous regressions :-)

Kamil

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Christopher Li]

On Thu, Sep 3, 2009 at 4:47 AM, Kamil Dudka<kdudka@redhat.com> wrote:
> Do you want to store the correct enum type to expression->ctype and overwrite
> it later with some integral type? What's the advantage of this approach?
> (instead of preserving ABI which is already changed to 0.4.1). I don't think
> the information about type for enum constant is useful only for issuing
> warnings like this. Some SPARSE clients might also need such information.

I think it is cleaner to have one single place to look for the ctype of
an expression.We don't have to convert it to int type. I think most of the
sparse code can deal with enum type any way. I don't like the dual personality
of expressions.

An other reason is that you increase the expression structure size for
32 bit system. Expression and symbol are the most common used
structure. It take up a good portion of the sparse memory foot print. I
want to keep them as lean as possible.

> Sure if anybody is able to split it... Unfortunately we have no reference for
> the "original" behavior of -Wmismatch-enum. I didn't found it documented
> anywhere and the "test-case" from the referred commit is not really useful as
> a test-case. I have not enough time for reverse engineering...

I don't see why it is hard to split. Can we just make the code which add
new options and issue new warnings to a separate patch? We shouldn't
need to reverse engineering the old behavior to do that.

>
> I think the tests included in the patch have good enough coverage to prevent
> you from a future regression and it sounds more important to me than
> documenting previous regressions :-)

Yes I agree the test case is always useful.

Chris

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

On Thursday 03 of September 2009 20:38:24 Christopher Li wrote:
> I think it is cleaner to have one single place to look for the ctype of
> an expression.We don't have to convert it to int type. I think most of the
> sparse code can deal with enum type any way. I don't like the dual
> personality of expressions.

Two personalities are still not so many ... have you ever looked to the gcc 
code? :-)

[...]

> I don't see why it is hard to split. Can we just make the code which add
> new options and issue new warnings to a separate patch? We shouldn't
> need to reverse engineering the old behavior to do that.

I didn't say hard, but actually not useful from my point of view. If it's easy 
for you to split my patch to the two pieces, you are welcome to have a go at 
that. We can still wait for opinion of the original authors. Perhaps they can 
bring some light to this problem.

Kamil

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Christopher Li]

On Thu, Sep 3, 2009 at 11:54 AM, Kamil Dudka<kdudka@redhat.com> wrote:
> Two personalities are still not so many ... have you ever looked to the gcc
> code? :-)

That is why I don't want to start a trend.

> I didn't say hard, but actually not useful from my point of view. If it's easy
> for you to split my patch to the two pieces, you are welcome to have a go at
> that. We can still wait for opinion of the original authors. Perhaps they can
> bring some light to this problem.

Fair enough .I need to spend more time to digest this patch.

Chris

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

On Thursday 03 of September 2009 22:02:06 Christopher Li wrote:
> On Thu, Sep 3, 2009 at 11:54 AM, Kamil Dudka<kdudka@redhat.com> wrote:
> > Two personalities are still not so many ... have you ever looked to the
> > gcc code? :-)
>
> That is why I don't want to start a trend.
>
> > I didn't say hard, but actually not useful from my point of view. If it's
> > easy for you to split my patch to the two pieces, you are welcome to have
> > a go at that. We can still wait for opinion of the original authors.
> > Perhaps they can bring some light to this problem.
>
> Fair enough .I need to spend more time to digest this patch.

Any progress here? I wouldn't like this to become another downstream patch.

I work on a generic layer [1] for processing C sources (for now) supporting 
SPARSE and gcc 4.5 plug-in as sources. The gcc plug-in [2] now works fairly 
well and I am going to make the SPARSE client fully equivalent to the 
plug-in. This will probably need a few more tiny patches for SPARSE
to implement what's missing.

Is the upstream interested to make SPARSE more useful as a library?

Kamil

[1]http://dudka.no-ip.org/cgi-bin/gitweb.cgi?p=tools;a=blob_plain;f=sl/code_listener.h
[2]http://dudka.no-ip.org/cgi-bin/gitweb.cgi?p=tools;a=blob_plain;f=sl/slplug.c

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Christopher Li]

On Sun, Sep 13, 2009 at 12:28 PM, Kamil Dudka <kdudka@redhat.com> wrote:
>
> Any progress here? I wouldn't like this to become another downstream patch.

Guilty. Too busy this week. I am going to take a look at it again tonight.

> I work on a generic layer [1] for processing C sources (for now) supporting
> SPARSE and gcc 4.5 plug-in as sources. The gcc plug-in [2] now works fairly
> well and I am going to make the SPARSE client fully equivalent to the
> plug-in. This will probably need a few more tiny patches for SPARSE
> to implement what's missing.
>
> Is the upstream interested to make SPARSE more useful as a library?

Yes. I am interested in that. If it does not cause additional slow down
or memory blow. Sure yes.

If it does because it needs to preserve more information, e.g. space and
comments it would be nice to put it under some options or even compile time
config option. I care about people don't use those feature can still run
sparse fast.

Chris

Re: [PATCH] add warnings enum-to-int and int-to-enum

[Kamil Dudka]

On Sunday 13 of September 2009 21:55:32 Christopher Li wrote:
> On Sun, Sep 13, 2009 at 12:28 PM, Kamil Dudka <kdudka@redhat.com> wrote:
> > Any progress here? I wouldn't like this to become another downstream
> > patch.
>
> Guilty. Too busy this week. I am going to take a look at it again tonight.

Thanks for quick reply! It doesn't hurry actually. I am only trying to keep 
this thread alive :-)

> > I work on a generic layer [1] for processing C sources (for now)
> > supporting SPARSE and gcc 4.5 plug-in as sources. The gcc plug-in [2] now
> > works fairly well and I am going to make the SPARSE client fully
> > equivalent to the plug-in. This will probably need a few more tiny
> > patches for SPARSE to implement what's missing.
> >
> > Is the upstream interested to make SPARSE more useful as a library?
>
> Yes. I am interested in that. If it does not cause additional slow down
> or memory blow. Sure yes.
>
> If it does because it needs to preserve more information, e.g. space and
> comments it would be nice to put it under some options or even compile time
> config option. I care about people don't use those feature can still run
> sparse fast.

I think we'll need to add some extra items to the public SPARSE headers, not 
sure if it's good idea to #ifdef it even there. But I need to analyse what 
exactly is missing first. Now I am only asking whether the idea itself is 
welcome (before starting work on it).

Kamil