Re: [PATCH V2 2/5] perf metricgroup: Factor out metricgroup__add_metric_weak_group()

[Arnaldo Carvalho de Melo]

Em Mon, Feb 24, 2020 at 01:59:21PM -0800, kan.liang@xxxxxxxxxxxxxxx escreveu:
> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> 
> Factor out metricgroup__add_metric_weak_group() which add metrics into a
> weak group. The change can improve code readability. Because following
> patch will introduce a function which add standalone metrics.
> 
> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> ---
>  tools/perf/util/metricgroup.c | 57 +++++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 02aee94..1cd042c 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -399,13 +399,42 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
>  	strlist__delete(metriclist);
>  }
>  
> +static void metricgroup__add_metric_weak_group(struct strbuf *events,
> +					       const char **ids,
> +					       int idnum)
> +{
> +	bool no_group = false;
> +	int i;
> +
> +	for (i = 0; i < idnum; i++) {
> +		pr_debug("found event %s\n", ids[i]);
> +		/*
> +		 * Duration time maps to a software event and can make
> +		 * groups not count. Always use it outside a
> +		 * group.
> +		 */
> +		if (!strcmp(ids[i], "duration_time")) {
> +			if (i > 0)
> +				strbuf_addf(events, "}:W,");
> +			strbuf_addf(events, "duration_time");

At some point we need to do some error checking on these strbuf calls,
but since this was the state in this file, lets not block this patchkit
on such grounds,

- Arnaldo

> +			no_group = true;
> +			continue;
> +		}
> +		strbuf_addf(events, "%s%s",
> +			i == 0 || no_group ? "{" : ",",
> +			ids[i]);
> +		no_group = false;
> +	}
> +	if (!no_group)
> +		strbuf_addf(events, "}:W");
> +}
> +
>  static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  				   struct list_head *group_list)
>  {
>  	struct pmu_events_map *map = perf_pmu__find_map(NULL);
>  	struct pmu_event *pe;
> -	int ret = -EINVAL;
> -	int i, j;
> +	int i, ret = -EINVAL;
>  
>  	if (!map)
>  		return 0;
> @@ -422,7 +451,6 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  			const char **ids;
>  			int idnum;
>  			struct egroup *eg;
> -			bool no_group = false;
>  
>  			pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>  
> @@ -431,27 +459,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
>  				continue;
>  			if (events->len > 0)
>  				strbuf_addf(events, ",");
> -			for (j = 0; j < idnum; j++) {
> -				pr_debug("found event %s\n", ids[j]);
> -				/*
> -				 * Duration time maps to a software event and can make
> -				 * groups not count. Always use it outside a
> -				 * group.
> -				 */
> -				if (!strcmp(ids[j], "duration_time")) {
> -					if (j > 0)
> -						strbuf_addf(events, "}:W,");
> -					strbuf_addf(events, "duration_time");
> -					no_group = true;
> -					continue;
> -				}
> -				strbuf_addf(events, "%s%s",
> -					j == 0 || no_group ? "{" : ",",
> -					ids[j]);
> -				no_group = false;
> -			}
> -			if (!no_group)
> -				strbuf_addf(events, "}:W");
> +
> +			metricgroup__add_metric_weak_group(events, ids, idnum);
>  
>  			eg = malloc(sizeof(struct egroup));
>  			if (!eg) {
> -- 
> 2.7.4
> 

-- 

- Arnaldo