Re: [PATCH 14/20] perf annotate: Pass struct annotation_options to map_symbol__annotation_dump

[Arnaldo Carvalho de Melo]

Em Mon, Aug 06, 2018 at 04:24:47PM +0200, Jiri Olsa escreveu:
> On Mon, Aug 06, 2018 at 10:45:07PM +0900, Namhyung Kim wrote:
> > Hi Jiri,
> > 
> > On Sat, Aug 04, 2018 at 03:05:15PM +0200, Jiri Olsa wrote:
> > > Passing struct annotation_options to map_symbol__annotation_dump,
> > > to carry on and pass the percent_type value.
> > > 
> > > Link: http://lkml.kernel.org/n/tip-5toohgdkgpk3vn6zebusr3bb@xxxxxxxxxxxxxx
> > > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > ---
> > 
> > [SNIP]
> > > @@ -2523,7 +2526,7 @@ static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> > >  
> > >  static void __annotation_line__write(struct annotation_line *al, struct annotation *notes,
> > >  				     bool first_line, bool current_entry, bool change_color, int width,
> > > -				     void *obj,
> > > +				     void *obj, unsigned int percent_type,
> > >  				     int  (*obj__set_color)(void *obj, int color),
> > >  				     void (*obj__set_percent_color)(void *obj, double percent, bool current),
> > >  				     int  (*obj__set_jumps_percent_color)(void *obj, int nr, bool current),
> > > @@ -2531,7 +2534,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > >  				     void (*obj__write_graph)(void *obj, int graph))
> > >  
> > >  {
> > > -	double percent_max = annotation_line__max_percent(al, notes);
> > > +	double percent_max = annotation_line__max_percent(al, notes, percent_type);
> > >  	int pcnt_width = annotation__pcnt_width(notes),
> > >  	    cycles_width = annotation__cycles_width(notes);
> > >  	bool show_title = false;
> > > @@ -2552,8 +2555,7 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > >  		for (i = 0; i < notes->nr_events; i++) {
> > >  			double percent;
> > >  
> > > -			percent = annotation_data__percent(&al->data[i],
> > > -							   PERCENT_HITS_LOCAL);
> > > +			percent = annotation_data__percent(&al->data[i], percent_type);
> > >  
> > >  			obj__set_percent_color(obj, percent, current_entry);
> > >  			if (notes->options->show_total_period) {
> > > @@ -2680,13 +2682,15 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > >  }
> > >  
> > >  void annotation_line__write(struct annotation_line *al, struct annotation *notes,
> > > -			    struct annotation_write_ops *ops)
> > > +			    struct annotation_write_ops *wops,
> > > +			    struct annotation_options *opts)
> > >  {
> > > -	__annotation_line__write(al, notes, ops->first_line, ops->current_entry,
> > > -				 ops->change_color, ops->width, ops->obj,
> > > -				 ops->set_color, ops->set_percent_color,
> > > -				 ops->set_jumps_percent_color, ops->printf,
> > > -				 ops->write_graph);
> > > +	__annotation_line__write(al, notes, wops->first_line, wops->current_entry,
> > > +				 wops->change_color, wops->width, wops->obj,
> > > +				 opts->percent_type,
> > > +				 wops->set_color, wops->set_percent_color,
> > > +				 wops->set_jumps_percent_color, wops->printf,
> > > +				 wops->write_graph);
> > 
> > This doesn't look good.  Why not just passing a pointer to wops
> > instead of each fields separately?
> 
> yep, my thoughts exactly when I saw this ;-) we probably had some

But then, while this is a valid observation, it is not related to this
patchkit, that is just adding an extra config variable, percent_type, at
some point one can try to shorten that function signature, looking at
why it was done this way originally to see if there was any reason or if
its just something to improve by shortening the function signature.

Applying Jiri's patch,

> other caller..  however I only wanted to add one more param ;-)

Right
 
> I'll check what we can do with this in v2

I'm going thru v2 already, so far its an uncontrovertial, trivial, so
I think you better just wait a teeny bit for this to be applied and then
get on over with followup patches, ok?

- Arnaldo