Re: [PATCH] perf metric: Code cleanup with map_for_each_event()

[liwei (GF)]

Hi Namhyung,

On 2020/9/17 13:19, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Sep 17, 2020 at 11:45 AM Wei Li <liwei391@xxxxxxxxxx> 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 <liwei391@xxxxxxxxxx>
> 
> Acked-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> 
> 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 for spotting that.
I'd like to move them just below match_metric() for now, when we really need
to use them somewhere else, we may move them to header file then.

Thanks,
Wei