From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754554AbbDTPXQ (ORCPT ); Mon, 20 Apr 2015 11:23:16 -0400 Received: from mail-pd0-f171.google.com ([209.85.192.171]:34349 "EHLO mail-pd0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750930AbbDTPXO (ORCPT ); Mon, 20 Apr 2015 11:23:14 -0400 Date: Tue, 21 Apr 2015 00:22:06 +0900 From: Namhyung Kim To: Arnaldo Carvalho de Melo Cc: Ingo Molnar , Peter Zijlstra , Jiri Olsa , LKML , David Ahern Subject: Re: [PATCH 6/7] perf hists browser: Split popup menu actions Message-ID: <20150420152206.GF8483@danjae.kornet> References: <1429416255-12070-1-git-send-email-namhyung@kernel.org> <1429416255-12070-7-git-send-email-namhyung@kernel.org> <20150420140020.GE11111@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150420140020.GE11111@kernel.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.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 > > --- > > 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