[Question] About '(*idx)++' of perf_evsel__new_idx

[Question] About '(*idx)++' of perf_evsel__new_idx

[Taeung Song]

Hi, :)

Can I ask you one thing ?

I'm reading source code util/parse-event.c
Along the way, I wonder why increase idx before checking
whether 'evsel' is NULL or not ? (at 310,311 line number)


300 static struct perf_evsel *
301 __add_event(struct list_head *list, int *idx,
302             struct perf_event_attr *attr,
303             char *name, struct cpu_map *cpus,
304             struct list_head *config_terms)
305 {
306         struct perf_evsel *evsel;
307
308         event_attr_init(attr);
309
310         evsel = perf_evsel__new_idx(attr, (*idx)++);
311         if (!evsel)
312                 return NULL;
313
314         evsel->cpus     = cpu_map__get(cpus);
315         evsel->own_cpus = cpu_map__get(cpus);
316

IMHO, if 'evsel' isn't NULL, we can increase idx like below.

             evsel = perf_evsel__new_idx(attr, *idx);
             if (!evsel)
                     return NULL;
             else
                (*idx)++;

Is it wrong ? or is there other reason about increasing idx
before check 'evsel'?

Thanks,
Taeung

Re: [Question] About '(*idx)++' of perf_evsel__new_idx

[Arnaldo Carvalho de Melo]

Em Mon, Jan 30, 2017 at 11:53:18AM +0900, Taeung Song escreveu:
> Hi, :)
> 
> Can I ask you one thing ?
> 
> I'm reading source code util/parse-event.c
> Along the way, I wonder why increase idx before checking
> whether 'evsel' is NULL or not ? (at 310,311 line number)
> 
> 
> 300 static struct perf_evsel *
> 301 __add_event(struct list_head *list, int *idx,
> 302             struct perf_event_attr *attr,
> 303             char *name, struct cpu_map *cpus,
> 304             struct list_head *config_terms)
> 305 {
> 306         struct perf_evsel *evsel;
> 307
> 308         event_attr_init(attr);
> 309
> 310         evsel = perf_evsel__new_idx(attr, (*idx)++);
> 311         if (!evsel)
> 312                 return NULL;
> 313
> 314         evsel->cpus     = cpu_map__get(cpus);
> 315         evsel->own_cpus = cpu_map__get(cpus);
> 316
> 
> IMHO, if 'evsel' isn't NULL, we can increase idx like below.
> 
>             evsel = perf_evsel__new_idx(attr, *idx);
>             if (!evsel)
>                     return NULL;
>             else
>                (*idx)++;
> 
> Is it wrong ? or is there other reason about increasing idx
> before check 'evsel'?

I think you're right and we should increment idx only if we manage to
create the evsel instance, no need for the else clause tho.

- Arnaldo

Re: [Question] About '(*idx)++' of perf_evsel__new_idx

[Taeung Song]

Evening! :)

On 01/31/2017 04:03 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 30, 2017 at 11:53:18AM +0900, Taeung Song escreveu:
>> Hi, :)
>>
>> Can I ask you one thing ?
>>
>> I'm reading source code util/parse-event.c
>> Along the way, I wonder why increase idx before checking
>> whether 'evsel' is NULL or not ? (at 310,311 line number)
>>
>>
>> 300 static struct perf_evsel *
>> 301 __add_event(struct list_head *list, int *idx,
>> 302             struct perf_event_attr *attr,
>> 303             char *name, struct cpu_map *cpus,
>> 304             struct list_head *config_terms)
>> 305 {
>> 306         struct perf_evsel *evsel;
>> 307
>> 308         event_attr_init(attr);
>> 309
>> 310         evsel = perf_evsel__new_idx(attr, (*idx)++);
>> 311         if (!evsel)
>> 312                 return NULL;
>> 313
>> 314         evsel->cpus     = cpu_map__get(cpus);
>> 315         evsel->own_cpus = cpu_map__get(cpus);
>> 316
>>
>> IMHO, if 'evsel' isn't NULL, we can increase idx like below.
>>
>>             evsel = perf_evsel__new_idx(attr, *idx);
>>             if (!evsel)
>>                     return NULL;
>>             else
>>                (*idx)++;
>>
>> Is it wrong ? or is there other reason about increasing idx
>> before check 'evsel'?
>
> I think you're right and we should increment idx only if we manage to
> create the evsel instance, no need for the else clause tho.
>

I understood!
Thank you!

Taeung