Re: [PATCH 00/15] tools: Unify perf and trace-cmd trace event format parsing v3

[Ingo Molnar <mingo@kernel.org>]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 2012-05-07 at 10:14 +0200, Ingo Molnar wrote:
> 
> > So can we please make a libevent.so, built sanely within 
> > tools/perf/lib/ or such and distributed together with perf so 
> > that the two can never get out of sync?
> 
> If you do this, you need to find a way to turn off -Wswitch-enum as that
> seems to (at least on my gcc) ignore defaults.
> 
> Thus we end up with:
> 
> diff --git a/Makefile b/Makefile
> index 18ad079..217548b 100644
> 
> diff --git a/parse-filter.c b/parse-filter.c
> index d09fbd2..0ac249c 100644
> --- a/parse-filter.c
> +++ b/parse-filter.c
> @@ -367,6 +367,12 @@ create_arg_item(struct event_format *event, const
> char *token,
>  		arg->type = FILTER_ARG_FIELD;
>  		arg->field.field = field;
>  		break;
> +	case EVENT_ERROR:
> +	case EVENT_NONE:
> +	case EVENT_SPACE:
> +	case EVENT_NEWLINE:
> +	case EVENT_OP:
> +	case EVENT_DELIM:
>  	default:
>  		free_arg(arg);
>  		show_error(error_str, "expected a value but found %s",

The advantage of this warning is that when a new enum value is 
added, every usage site is forced to consider it.

Code not using it indeed needs to be updated.

> Looking at what was done in the current code, there's lots of :
> 
> 	case PRINT_NULL:
> 	case PRINT_FIELD ... PRINT_SYMBOL:
> 	case PRINT_STRING:
> 	default:
> 
> Which looks more of a maintenance nightmare. How does this help? The
> FOO ... BAR, will hide the same errors that you are trying to prevent.

Indeed the FOO...BAR pattern should not be used (i.e. the above 
is a bug in essence), unless it's clearly some genuine same-type 
numeric range that is expressed, where new fields can never get 
inbetween.

> If you were suppose to handle something between FOO and BAR, then you
> just ignored it too.

Correct.

> It also makes that "default" redundant.

Yes.

> This is one of those warnings that causes more pain than it 
> helps.

I disagree, when I switched to it then it found some real bugs 
and new changes were incremental.

> If you strongly believe that all these warnings are helpful, 
> than we should push to add these warnings to the kernel too.

For subsystems I maintain we could consider it - but for the 
kernel as a whole it would be quite some work.

Thanks,

	Ingo