lkml.org 
[lkml]   [2015]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 6/7] perf hists browser: Split popup menu actions
Hi Arnaldo,

On Mon, Apr 20, 2015 at 11:00:20AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Apr 19, 2015 at 01:04:14PM +0900, Namhyung Kim escreveu:
> > Currently perf_evsel__hists_browse() function spins on a huge loop and
> > handles many key actions. Since it's hard to read and modify, let's
> > split it out into small helper functions.
> >
> > The add_XXX_opt() functions are to register popup menu item on the
> > selected entry. When it adds an item, it also saves related data into
> > struct popup_option and returns 1 so that it can increase the number of
> > items (nr_opts). A callback function named do_XXX is called with saved
> > data when the item is selected by user.
> >
> > No functional change intended.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/ui/browsers/hists.c | 565 ++++++++++++++++++++++++++---------------
> > 1 file changed, 363 insertions(+), 202 deletions(-)
> >
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index cace2df7e561..315ebc493508 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -1216,11 +1216,6 @@ static void hist_browser__delete(struct hist_browser *browser)
> > free(browser);
> > }
> >
> > -static struct hist_entry *hist_browser__selected_entry(struct hist_browser *browser)
> > -{
> > - return browser->he_selection;
> > -}
> > -
>
> Why remove the above function? To reduce the patch size you could have
> left it and if the reason for removing it is that compelling, remove it
> in a later patch.

OK, will do.

>
> > static struct thread *hist_browser__selected_thread(struct hist_browser *browser)
> > {
> > return browser->he_selection->thread;
> > @@ -1395,6 +1390,281 @@ close_file_and_continue:
> > return ret;
> > }
> >
> > +struct popup_option {
> > + struct thread *thread;
> > + struct map *map;
> > + struct dso *dso;
> > + struct symbol *sym;
>
> You could use struct map_symbol, that has the three above, right? In
> some cases you would have less lines by doing:
>
>
> ms = po->ms;

OK.

>
> > + int (*fn)(struct popup_option *opt, struct hist_browser *browser,
> > + struct hist_browser_timer *hbt, struct pstack *pstack,
>
> I wonder if, as a prep patch, you couldn't have browser->hbt, so that we
> would reduce the function signature above. Ditto for pstack.

I don't get it. The hbt and pstack is needed to annotate and zoom.
Are you saying about step-by-step conversion for each action like
first patch for annotate, seconf for zoom, and so on..?

>
>
> Also, you're adding the mechanism (popup_option) at the same time that
> you make those new functions use it.
>
> I think that it would be better if you first created the functions, call
> them directly, then introduce popup_option (probably better named as
> "popup_action").
>
> Each patch would be smaller and more easily reviewable, as well bisect
> would work better when locating bugs.

OK, I'll split the patch into steps..

>
> > + struct perf_session_env *env);
> > +};
> > +
> > +static int
> > +do_annotate(struct popup_option *opt, struct hist_browser *browser,
> > + struct hist_browser_timer *hbt, struct pstack *pstack __maybe_unused,
> > + struct perf_session_env *env)
> > +{
> > + struct perf_evsel *evsel;
> > + struct annotation *notes;
> > + struct map_symbol ms;
> > + int err;
> > +
> > + if (!objdump_path && perf_session_env__lookup_objdump(env))
> > + return 0;
> > +
> > + ms.map = opt->map;
> > + ms.sym = opt->sym;
> > +
> > + notes = symbol__annotation(ms.sym);
> > + if (!notes->src)
> > + return 0;
> > +
> > + evsel = hists_to_evsel(browser->hists);
> > + err = map_symbol__tui_annotate(&ms, evsel, hbt);
> > + /*
> > + * offer option to annotate the other branch source or target
> > + * (if they exists) when returning from annotate
> > + */
> > + if ((err == 'q' || err == CTRL('c'))
> > + && browser->he_selection->branch_info)
>
> Please put the && operator at the end of the first line, even if
> originally it was like that (was it?).

Yes, it was. I'll change it anyway..

Thanks,
Namhyung


\
 
 \ /
  Last update: 2015-04-20 18:01    [W:0.087 / U:4.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site