| Message ID | 1438689994-30940-1-git-send-email-tcamuso@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Aug 4, 2015 at 8:06 AM, Tony Camuso <tcamuso@redhat.com> wrote: > Missing enum members in case statements in c2xml.c and parse.c were > causing compile time complaints by gcc 5.1.1. Better to explicitly > handle all enum members of the switch variable in case statements, > even if they just break. > break; > + case SYM_PREPROCESSOR: > + case SYM_BASETYPE: > + case SYM_NODE: > + case SYM_PTR: > + case SYM_ARRAY: > + case SYM_ENUM: > + case SYM_TYPEDEF: > + case SYM_TYPEOF: > + case SYM_MEMBER: > + case SYM_BITFIELD: > + case SYM_LABEL: > + case SYM_RESTRICT: > + case SYM_FOULED: > + case SYM_KEYWORD: > + case SYM_BAD: > + break; I think adding a "default: break" should work. I like your previous patch better, except maybe adding a line break after default. > case SYM_ENUM: > case SYM_RESTRICT: > base_type->ident = ident; > + case SYM_PREPROCESSOR: > + case SYM_BASETYPE: I notice this has the follow through behavior. It is not a bug. But if some one append some new case there, it is easy to cause unintended follow through. It is better add a break there. 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 08/04/2015 07:31 PM, Christopher Li wrote: > On Tue, Aug 4, 2015 at 8:06 AM, Tony Camuso <tcamuso@redhat.com> wrote: >> Missing enum members in case statements in c2xml.c and parse.c were >> causing compile time complaints by gcc 5.1.1. Better to explicitly >> handle all enum members of the switch variable in case statements, >> even if they just break. >> break; >> + case SYM_PREPROCESSOR: >> + case SYM_BASETYPE: >> + case SYM_NODE: >> + case SYM_PTR: >> + case SYM_ARRAY: >> + case SYM_ENUM: >> + case SYM_TYPEDEF: >> + case SYM_TYPEOF: >> + case SYM_MEMBER: >> + case SYM_BITFIELD: >> + case SYM_LABEL: >> + case SYM_RESTRICT: >> + case SYM_FOULED: >> + case SYM_KEYWORD: >> + case SYM_BAD: >> + break; > > I think adding a "default: break" should work. I like your previous patch > better, except maybe adding a line break after default. > It was mentioned to me that the new prevailing wisdom is to "Prefer compile time errors to runtime errors", such that each enum'd value of a switch variable should be explicitly handled. I seem to have violated the spirit of that paradigm by using fall through. I could add a new line and "break" statement after each case. Or, if you prefer, I can resubmit the original patch with a linefeed after the "default:" for good form. Either way, I believe the compiler optimize them the same. > >> case SYM_ENUM: >> case SYM_RESTRICT: >> base_type->ident = ident; >> + case SYM_PREPROCESSOR: >> + case SYM_BASETYPE: > > I notice this has the follow through behavior. It is not a bug. > But if some one append some new case there, it is easy to > cause unintended follow through. It is better add a break there. It may not be a bug today, but without an explicit break for each, it's a potential bug. The whole reason for explicitly listing the existing enum members is so that new cases for which "break" may not be the correct solution can be easily identified at compile time. Tony -- 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 08/07/2015 11:47 PM, Christopher Li wrote: > > > On Tue, Aug 4, 2015 at 7:52 PM, Tony Camuso <tcamuso@redhat.com <mailto:tcamuso@redhat.com>> wrote: > > > > It was mentioned to me that the new prevailing wisdom is to "Prefer > > compile time errors to runtime errors", such that each enum'd value > > of a switch variable should be explicitly handled. > > Most of the code I saw, when using fail through, has comment emphasizing > the fall through behavior. > > > > Or, if you prefer, I can resubmit the original patch with a linefeed after > > the "default:" for good form. Either way, I believe the compiler optimize > > them the same. > > Yes, I prefer the patch without have to list every enum member just to > skip them. > > > The whole reason for explicitly listing the existing enum members is so > > that new cases for which "break" may not be the correct solution can be > > easily identified at compile time. > > I can see that. However it only works for the switch statement that does > not have the default handler already. In the field a lot of the switch statements > already have default handler, depending on the compiler to give warning is > not that reliable. Unless we ban the default handler all together. > > Chris > Thanks Chris, I will resubmit the original patch with your recommendation for a newline after the default: case. Regards, Tony -- 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/c2xml.c b/c2xml.c index 67486c7..a53f5bd 100644 --- a/c2xml.c +++ b/c2xml.c @@ -214,6 +214,22 @@ static void examine_symbol(struct symbol *sym, xmlNodePtr node) case SYM_UNINITIALIZED: newProp(child, "base-type-builtin", builtin_typename(sym)); break; + case SYM_PREPROCESSOR: + case SYM_BASETYPE: + case SYM_NODE: + case SYM_PTR: + case SYM_ARRAY: + case SYM_ENUM: + case SYM_TYPEDEF: + case SYM_TYPEOF: + case SYM_MEMBER: + case SYM_BITFIELD: + case SYM_LABEL: + case SYM_RESTRICT: + case SYM_FOULED: + case SYM_KEYWORD: + case SYM_BAD: + break; } return; } diff --git a/parse.c b/parse.c index b43d683..74a56e4 100644 --- a/parse.c +++ b/parse.c @@ -2769,6 +2769,21 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis case SYM_ENUM: case SYM_RESTRICT: base_type->ident = ident; + 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) {
Missing enum members in case statements in c2xml.c and parse.c were causing compile time complaints by gcc 5.1.1. Better to explicitly handle all enum members of the switch variable in case statements, even if they just break. Signed-off-by: Tony Camuso <tcamuso@redhat.com> --- c2xml.c | 16 ++++++++++++++++ parse.c | 15 +++++++++++++++ 2 files changed, 31 insertions(+)