Messages in this thread | | | Date | Mon, 20 Apr 2015 18:28:45 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 6/7] perf hists browser: Split popup menu actions |
| |
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 <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..?
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 *).
- Arnaldo
| |