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

[Jiri Olsa]

On Wed, Sep 21, 2016 at 12:14:32PM -0300, Arnaldo Carvalho de Melo wrote:
> 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@xxxxxxxxxxxxxx
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
> >  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.

ook, will change

thanks,
jirka