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-j2sizkyglam3narmndlj9...@git.kernel.org
> > Signed-off-by: Jiri Olsa 
> > ---
> >  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

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-j2sizkyglam3narmndlj9...@git.kernel.org
> > Signed-off-by: Jiri Olsa 
> > ---
> >  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

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

[Arnaldo Carvalho de Melo]

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-j2sizkyglam3narmndlj9...@git.kernel.org
> Signed-off-by: Jiri Olsa 
> ---
>  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, , hists, fp);
>  
> - hist_entry__snprintf(he, );
> + hist_entry__snprintf(he, , hists->hpp_list);
>  
>   ret = fprintf(fp, "%s\n", bf);
>  
> -- 
> 2.7.4

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

[Arnaldo Carvalho de Melo]

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-j2sizkyglam3narmndlj9...@git.kernel.org
> Signed-off-by: Jiri Olsa 
> ---
>  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, , hists, fp);
>  
> - hist_entry__snprintf(he, );
> + hist_entry__snprintf(he, , hists->hpp_list);
>  
>   ret = fprintf(fp, "%s\n", bf);
>  
> -- 
> 2.7.4