| Message ID | 4A663005.5090009@ramsay1.demon.co.uk (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Tue, Jul 21, 2009 at 2:15 PM, Ramsay Jones<ramsay@ramsay1.demon.co.uk> wrote: >> I don't think you want to add defaults like this just to avoid warnings. >> Warnings like that can help when adding a new item to an enum, to find >> the places where you need to extend the code to hand the new item. Â And >> since current GCC doesn't even issue the warning by default, it seems >> even more unnecessary to add that default case. >> > > OK... > > So, if I understand your argument, in order to make the best use of these > warnings, then the correct change would look like the diff given below, > and (for more up-to-date gcc) add -Wswitch-enum to CFLAGS (at least > occasionally). OK. I don't want to list all the enumerate value here just for the sake of gcc warnings. It makes the code ugly. Nor do I want to change the gcc flags used to compile sparse. If the newest gcc still complain on those. I would rather add the blank default to make it clean. Since latest gcc doesn't issue warning on those. I think it is fine to leave it as it is. 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
On Tue, Jul 21, 2009 at 10:15:49PM +0100, Ramsay Jones wrote: > Josh Triplett wrote: > > On Sat, Jul 18, 2009 at 09:41:46PM +0100, Ramsay Jones wrote: > >> These warnings were issued by gcc v3.4.4, but not by gcc v4.1.2. > >> So I guess gcc probably found these warnings to be too noisy ... > > [...] > >> --- a/parse.c > >> +++ b/parse.c > >> @@ -2616,6 +2616,8 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis > >> case SYM_ENUM: > >> case SYM_RESTRICT: > >> base_type->ident = ident; > >> + default: > >> + break; > >> } > > > > I don't think you want to add defaults like this just to avoid warnings. > > Warnings like that can help when adding a new item to an enum, to find > > the places where you need to extend the code to hand the new item. And > > since current GCC doesn't even issue the warning by default, it seems > > even more unnecessary to add that default case. > > > > OK... > > So, if I understand your argument, in order to make the best use of these > warnings, then the correct change would look like the diff given below, > and (for more up-to-date gcc) add -Wswitch-enum to CFLAGS (at least > occasionally). [...] > diff --git a/parse.c b/parse.c > index e5ad867..afe39c3 100644 > --- a/parse.c > +++ b/parse.c > @@ -2616,6 +2616,23 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis > case SYM_ENUM: > case SYM_RESTRICT: > base_type->ident = ident; > + break; > + case SYM_UNINITIALIZED: > + case SYM_PREPROCESSOR: > + case SYM_BASETYPE: > + case SYM_NODE: > + case SYM_PTR: > + case SYM_FN: > + case SYM_ARRAY: > + case SYM_TYPEDEF: > + case SYM_TYPEOF: > + case SYM_MEMBER: > + case SYM_BITFIELD: > + case SYM_LABEL: > + case SYM_FOULED: > + case SYM_KEYWORD: > + case SYM_BAD: > + break; This represents exactly why more recent GCC defaults this warning to off. ;) No, I don't think writing the code this way helps. I think it makes more sense to leave the warning off and not bother placating older versions of GCC. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christopher Li wrote: > OK. I don't want to list all the enumerate value here just for the > sake of gcc warnings. > It makes the code ugly. Nor do I want to change the gcc flags used to compile > sparse. If the newest gcc still complain on those. I would rather add > the blank default > to make it clean. Since latest gcc doesn't issue warning on those. I > think it is fine to > leave it as it is. > OK, no problem. I can maintain this in my cygwin repo, along with the __sentinel__ attribute patch. (As before, I don't need this on Linux). Just FYI, I had planned to fix this by implementing a local makefile config file (like config.mak in git), so that I could fix this issue locally ("out of tree"). However, I was (pleasantly) surprised to find that I'd been beaten to the punch by Samuel Bronson in commit 8d86d0e. So I tried this local.mk file: OS=cygwin CFLAGS+=-Wno-switch-enum This lead to the invocation of gcc I was hoping for; something like: gcc ... -Wall ... -Wno-switch-enum parse.c which I had expected to suppress the warnings. It didn't. *Ho-Hum* ;-) ATB, Ramsay Jones -- 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
On Fri, Jul 24, 2009 at 11:25 AM, Ramsay Jones<ramsay@ramsay1.demon.co.uk> wrote: > Just FYI, I had planned to fix this by implementing a local makefile > config file (like config.mak in git), so that I could fix this issue > locally ("out of tree"). > > However, I was (pleasantly) surprised to find that I'd been beaten to > the punch by Samuel Bronson in commit 8d86d0e. > > So I tried this local.mk file: > > Â Â OS=cygwin > Â Â CFLAGS+=-Wno-switch-enum > > This lead to the invocation of gcc I was hoping for; something like: > > Â Â gcc ... -Wall ... -Wno-switch-enum parse.c Yes, that works. I try the latest version of the cygwin. The gcc in cygwin is still old enough to give the warning. I am going to leave it as it is. BTW, you can put some local build target in local.mk as well, not just config changes. 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
diff --git a/parse.c b/parse.c index e5ad867..afe39c3 100644 --- a/parse.c +++ b/parse.c @@ -2616,6 +2616,23 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis case SYM_ENUM: case SYM_RESTRICT: base_type->ident = ident; + break; + case SYM_UNINITIALIZED: + case SYM_PREPROCESSOR: + case SYM_BASETYPE: + case SYM_NODE: + case SYM_PTR: + case SYM_FN: + case SYM_ARRAY: + case SYM_TYPEDEF: + case SYM_TYPEOF: + case SYM_MEMBER: + case SYM_BITFIELD: + case SYM_LABEL: + case SYM_FOULED: + case SYM_KEYWORD: + case SYM_BAD: + break; } } } else if (base_type && base_type->type == SYM_FN) {