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
From 381f635e581c3d346ae47672dc7be1423b681924 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
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;
}
