Hi Arnaldo,
On Mon, Apr 20, 2015 at 06:28:45PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 21, 2015 at 12:22:06AM +0900, Namhyung Kim escreveu:
> > 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 <namhy...@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..?
>
> I am saying that pstack was a local variable in that big function,
> instead, perhaps we can have it as a member of struct hists_browser, so
> that we don't have to pass (struct hist_browser *, struct pstack *),
> just (struct hist_browser *).
Ah, I got it. Will change that way.
Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
