Re: [PATCH 6/7] perf hists browser: Split popup menu actions

[Namhyung Kim <namhyung@kernel.org>]

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