Re: [tip:perfcounters/core] perf: Enable more compiler warnings

Ingo Molnar
2009-08-16T14:02:13Z

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Sun, Aug 16, 2009 at 08:57:54AM +0000, tip-bot for Ingo Molnar wrote:
> > Commit-ID: 83a0944fa919fb2ebcfc1f8933d86e437b597ca6
> > Gitweb: http://git.kernel.org/tip/83a0944fa919fb2ebcfc1f8933d86e437b597ca6
> > Author: Ingo Molnar <mingo@elte.hu>
> > AuthorDate: Sat, 15 Aug 2009 12:26:57 +0200
> > Committer: Ingo Molnar <mingo@elte.hu>
> > CommitDate: Sun, 16 Aug 2009 10:47:47 +0200
> >
> > perf: Enable more compiler warnings
> >
> > Related to a shadowed variable bug fix Valdis Kletnieks noticed
> > that perf does not get built with -Wshadow, which could have
> > helped us avoid the bug.
> >
> > So enable -Wshadow and also enable the following warnings on
> > perf builds, in addition to the already enabled -Wall -Wextra
> > -std=gnu99 warnings:
> >
> > -Wcast-align
> > -Wformat=2
> > -Wshadow
> > -Winit-self
> > -Wpacked
> > -Wredundant-decls
> > -Wstack-protector
> > -Wstrict-aliasing=3
> > -Wswitch-default
> > -Wswitch-enum
>
>
>
> This one looks like more a pain than something useful.
>
> I have this code in perf trace:
>
> static int event_item_type(enum event_type type)
> {
> switch (type) {
> case EVENT_ITEM:
> case EVENT_DQUOTE:
> case EVENT_SQUOTE:
> return 1;
> default:
> return 0;
> }
> }
>
> Which results in:
>
> util/trace-event-parse.c: In function ‘event_item_type’:
> util/trace-event-parse.c:377: erreur: enumeration value ‘EVENT_ERROR’ not handled in switch
> util/trace-event-parse.c:377: erreur: enumeration value ‘EVENT_NONE’ not handled in switch
> util/trace-event-parse.c:377: erreur: enumeration value ‘EVENT_SPACE’ not handled in switch
> util/trace-event-parse.c:377: erreur: enumeration value ‘EVENT_NEWLINE’ not handled in switch
> util/trace-event-parse.c:377: erreur: enumeration value ‘EVENT_OP’ not handled in switch
> util/trace-event-parse.c:377: erreur: enumeration value ‘EVENT_DELIM’ not handled in switch
>
> The default case already handles these and I guess we don't want workarounds like:
>
> static int event_item_type(enum event_type type)
> {
> switch (type) {
> case EVENT_ITEM:
> case EVENT_DQUOTE:
> case EVENT_SQUOTE:
> return 1;
> case EVENT_ERROR:
> case EVENT_NONE:
> case EVENT_SPACE:
> case EVENT_NEWLINE:
> case EVENT_OP:
> case EVENT_DELIM:
> default:
> return 0;
> }
> }
>
>
> Right? :-)
>
> This warning might be useful for *very* specific cases, but not here IMO.

i think it's useful when an enum gets extended. Say we add a new
event enum and want to make sure it's handled in _all_ switch
statements that deals with them. This warning ensures that we
propagate the new case to all usage sites.

Ingo