Re: [RFC PATCH v2 10/13] perf metricgroup: Split up metricgroup__add_metric()

[John Garry]

On 22/04/2020 12:44, Jiri Olsa wrote:
> >   static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> >   				   struct list_head *group_list)
> >   {
> > @@ -502,37 +542,12 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> >   			break;
> >   		if (!pe->metric_expr)
> >   			continue;
> > -		if (match_metric(pe->metric_group, metric) ||
> > -		    match_metric(pe->metric_name, metric)) {
> > -			const char **ids;
> > -			int idnum;
> > -			struct egroup *eg;
> > -
> > -			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
> >   
> > -			if (expr__find_other(pe->metric_expr,
> > -					     NULL, &ids, &idnum) < 0)
> > -				continue;
> > -			if (events->len > 0)
> > -				strbuf_addf(events, ",");
> > -
> > -			if (metricgroup__has_constraint(pe))
> > -				metricgroup__add_metric_non_group(events, ids, idnum);
> > -			else
> > -				metricgroup__add_metric_weak_group(events, ids, idnum);
> > -
> > -			eg = malloc(sizeof(struct egroup));
> > -			if (!eg) {
> > -				ret = -ENOMEM;
> > -				break;
> > -			}
> > -			eg->ids = ids;
> > -			eg->idnum = idnum;
> > -			eg->metric_name = pe->metric_name;
> > -			eg->metric_expr = pe->metric_expr;
> > -			eg->metric_unit = pe->unit;
> > -			list_add_tail(&eg->nd, group_list);
> > -			ret = 0;
> also this place got changed just recently a lot,
> so you might want to rebase to the Arnaldo's latest perf/core

Hi jirka,

Yeah, I saw that. I can check.

TBH, apart from that, I would be welcome to opinion on this latter patch 
of the series, concerned with metrics. I just split (butcher) the 
function and call common parts from 2x places now. Maybe there's a more 
fluid way to do this.

Cheers,
John