Hello, On Thu, Sep 17, 2020 at 11:45 AM Wei Li <liwei...@huawei.com> wrote: > > Since we have introduced map_for_each_event() to walk the 'pmu_events_map', > clean up metricgroup__print() and metricgroup__has_metric() with it. > > Signed-off-by: Wei Li <liwei...@huawei.com>
Acked-by: Namhyung Kim <namhy...@kernel.org> A nit-pick below: > --- > tools/perf/util/metricgroup.c | 33 +++++++++++++-------------------- > 1 file changed, 13 insertions(+), 20 deletions(-) > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > index 8831b964288f..3734cbb2c456 100644 > --- a/tools/perf/util/metricgroup.c > +++ b/tools/perf/util/metricgroup.c > @@ -26,6 +26,17 @@ > #include "util.h" > #include <asm/bug.h> > > +#define map_for_each_event(__pe, __idx, __map) \ > + for (__idx = 0, __pe = &__map->table[__idx]; \ > + __pe->name || __pe->metric_group || __pe->metric_name; \ > + __pe = &__map->table[++__idx]) > + > +#define map_for_each_metric(__pe, __idx, __map, __metric) \ > + map_for_each_event(__pe, __idx, __map) \ > + if (__pe->metric_expr && \ > + (match_metric(__pe->metric_group, __metric) || \ > + match_metric(__pe->metric_name, __metric))) > + You may consider adding a declaration of match_metric() here. Right now, all users are below the function so it's ok but having the macro here can enable future addition above IMHO. Thanks Namhyung > struct metric_event *metricgroup__lookup(struct rblist *metric_events, > struct evsel *evsel, > bool create) > @@ -475,12 +486,9 @@ void metricgroup__print(bool metrics, bool metricgroups, > char *filter, > groups.node_new = mep_new; > groups.node_cmp = mep_cmp; > groups.node_delete = mep_delete; > - for (i = 0; ; i++) { > + map_for_each_event(pe, i, map) { > const char *g; > - pe = &map->table[i]; > > - if (!pe->name && !pe->metric_group && !pe->metric_name) > - break; > if (!pe->metric_expr) > continue; > g = pe->metric_group; > @@ -745,17 +753,6 @@ static int __add_metric(struct list_head *metric_list, > return 0; > } > > -#define map_for_each_event(__pe, __idx, __map) \ > - for (__idx = 0, __pe = &__map->table[__idx]; \ > - __pe->name || __pe->metric_group || __pe->metric_name; \ > - __pe = &__map->table[++__idx]) > - > -#define map_for_each_metric(__pe, __idx, __map, __metric) \ > - map_for_each_event(__pe, __idx, __map) \ > - if (__pe->metric_expr && \ > - (match_metric(__pe->metric_group, __metric) || \ > - match_metric(__pe->metric_name, __metric))) > - > static struct pmu_event *find_metric(const char *metric, struct > pmu_events_map *map) > { > struct pmu_event *pe; > @@ -1092,11 +1089,7 @@ bool metricgroup__has_metric(const char *metric) > if (!map) > return false; > > - for (i = 0; ; i++) { > - pe = &map->table[i]; > - > - if (!pe->name && !pe->metric_group && !pe->metric_name) > - break; > + map_for_each_event(pe, i, map) { > if (!pe->metric_expr) > continue; > if (match_metric(pe->metric_name, metric)) > -- > 2.17.1 >