Re: [PATCH 03/61] perf tools: Make hist_entry__snprintf work over struct perf_hpp_list

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Jiri Olsa <jolsa@kernel.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
	Don Zickus <dzickus@redhat.com>, Joe Mario <jmario@redhat.com>,
	Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Namhyung Kim <namhyung@kernel.org>,
	David Ahern <dsahern@gmail.com>, Andi Kleen <andi@firstfloor.org>
Subject: Re: [PATCH 03/61] perf tools: Make hist_entry__snprintf work over struct perf_hpp_list
Date: Wed, 21 Sep 2016 12:14:32 -0300	[thread overview]
Message-ID: <20160921151432.GG4973@kernel.org> (raw)
In-Reply-To: <1474290610-23241-4-git-send-email-jolsa@kernel.org>

Em Mon, Sep 19, 2016 at 03:09:12PM +0200, Jiri Olsa escreveu:
> Make hist_entry__snprintf to take perf_hpp_list as an argument
> instead of using he->hists->hpp_list. This way we can display
> arbitrary list of entries regardles of the hists setup, which
> will be useful in following patches.
> 
> Link: http://lkml.kernel.org/n/tip-j2sizkyglam3narmndlj99xq@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/ui/stdio/hist.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
> index a57131e61fe3..cb0371106c21 100644
> --- a/tools/perf/ui/stdio/hist.c
> +++ b/tools/perf/ui/stdio/hist.c
> @@ -373,7 +373,8 @@ static size_t hist_entry_callchain__fprintf(struct hist_entry *he,
>  	return 0;
>  }
>  
> -static int hist_entry__snprintf(struct hist_entry *he, struct perf_hpp *hpp)
> +static int hist_entry__snprintf(struct hist_entry *he, struct perf_hpp *hpp,
> +				struct perf_hpp_list *hpp_list)

What I usually do in these cases is to keep the existing interface and
add a new one that is more low level, that exhibits more flexibility,
i.e.:

static int __hist_entry__snprintf(struct hist_entry *he, struct perf_hpp *hpp,
				  struct perf_hpp_list *hpp_list)
{
...
}

And:

static int hist_entry__snprintf(struct hist_entry *he, struct perf_hpp *hpp)
{
	return __hist_entry__snprintf(he, hpp, he->hists->hpp_list);
}

This way no users of the existing function need to be changed, and new
ones can use the more flexible, lower level interface.

In this case there is just one such user, but the refactoring technique
could be consistently used, other people will not be left scratching
their heads asking why we pass something that can be obtained from
another parameter already in that function, while __ functions already
indicate they are more flexible and thus can flout that assumption.

- Arnaldo

>  {
>  	const char *sep = symbol_conf.field_sep;
>  	struct perf_hpp_fmt *fmt;
> @@ -384,7 +385,7 @@ static int hist_entry__snprintf(struct hist_entry *he, struct perf_hpp *hpp)
>  	if (symbol_conf.exclude_other && !he->parent)
>  		return 0;
>  
> -	hists__for_each_format(he->hists, fmt) {
> +	perf_hpp_list__for_each_format(hpp_list, fmt) {
>  		if (perf_hpp__should_skip(fmt, he->hists))
>  			continue;
>  
> @@ -509,7 +510,7 @@ static int hist_entry__fprintf(struct hist_entry *he, size_t size,
>  	if (symbol_conf.report_hierarchy)
>  		return hist_entry__hierarchy_fprintf(he, &hpp, hists, fp);
>  
> -	hist_entry__snprintf(he, &hpp);
> +	hist_entry__snprintf(he, &hpp, hists->hpp_list);
>  
>  	ret = fprintf(fp, "%s\n", bf);
>  
> -- 
> 2.7.4