From patchwork Wed Sep 2 11:53:17 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kamil Dudka X-Patchwork-Id: 45180 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n82BrqJ2009475 for ; Wed, 2 Sep 2009 11:53:52 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751840AbZIBLxs (ORCPT ); Wed, 2 Sep 2009 07:53:48 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751906AbZIBLxs (ORCPT ); Wed, 2 Sep 2009 07:53:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58786 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751840AbZIBLxr (ORCPT ); Wed, 2 Sep 2009 07:53:47 -0400 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n82BqE2a023707; Wed, 2 Sep 2009 07:52:14 -0400 Received: from dhcp-lab-205.englab.brq.redhat.com (dhcp-lab-205.englab.brq.redhat.com [10.34.33.205]) by int-mx08.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id n82BqDdq021842; Wed, 2 Sep 2009 07:52:14 -0400 From: Kamil Dudka To: Josh Triplett , Stephen Hemminger Subject: Re: [PATCH] add warnings enum-to-int and int-to-enum Date: Wed, 2 Sep 2009 13:53:17 +0200 User-Agent: KMail/1.12.0 (Linux/2.6.29.5-191.fc11.x86_64; KDE/4.3.0; x86_64; ; ) Cc: linux-sparse@vger.kernel.org References: <20090830153202.4dc5c58c@s6510> <200909012359.09678.kdudka@redhat.com> <20090901232433.GC3947@josh-work.beaverton.ibm.com> In-Reply-To: <20090901232433.GC3947@josh-work.beaverton.ibm.com> MIME-Version: 1.0 X-Length: 14932 X-UID: 47580 Message-Id: <200909021353.17091.kdudka@redhat.com> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.21 Sender: linux-sparse-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-sparse@vger.kernel.org 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 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 --- 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