Re: [PATCH 07/10] perf script: Move printing of 'common' data fromprint_event and rename

[Frederic Weisbecker]

On Wed, Mar 09, 2011 at 05:32:22PM -0700, David Ahern wrote:
> 
> 
> On 03/09/11 17:22, Frederic Weisbecker wrote:
> > On Wed, Mar 09, 2011 at 05:15:00PM -0700, David Ahern wrote:
> >>
> >>
> >> On 03/09/11 17:10, Frederic Weisbecker wrote:
> >>>
> >>> And if you actually keep those functions in place?
> >>
> >>     CC /tmp/build-perf/util/trace-event-read.o
> >> cc1: warnings being treated as errors
> >> util/trace-event-parse.c:2652:13: error: 'print_graph_cpu' defined but
> >> not used
> >> util/trace-event-parse.c:2681:13: error: 'print_graph_proc' defined but
> >> not used
> >> make: *** [/tmp/build-perf/util/trace-event-parse.o] Error 1
> >> make: *** Waiting for unfinished jobs....
> > 
> > All right, that's because you removed their calls in pretty_print_func_ent().
> > So either you remove the whole in a specific patch, pretty_print_func_ent()
> > included and other related functions, or you keep them.
> > 
> > But I prefer we don't do something halfway, and in particular not in a
> > semi-hidden way inside a patch that is not particularly focused on that
> > purpose.
> 
> You lost me on the halfway part.
> 
> So you want a separate patch that removes the code for an incomplete
> feature -- which means changing the references to the functions in that
> patch?

Yep.

> The intent being a patch that can be reverted later?

That can be reverted or something on top of which we can refer to
get that feature later.

In any case it is deadcode right now. If you remove it it should be
better done seperately. For all the reasons we always prefer to have
patches that do only one logic thing: easier to review, understand,
bisect, revert, find a bug, explain it, etc...

Ok, in fact you can actually keep it like you did: remove the cpu and
proc displays and let the rest of the function graph features. But
at least do it in an isolated patch because that should not be
an unnoticeable change, and for other reasons to make it a standalone
patch quoted above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/