On Mon, Jun 01, 2020 at 12:44:15AM -0700, Ian Rogers wrote: SNIP
> > + memset(&error, 0, sizeof(error)); > > + ret = parse_events_fake(evlist, id, &error); > > + if (ret) { > > + pr_debug("str : %s\n", error.str); > > + pr_debug("help : %s\n", error.help); > > + pr_debug("first_str : %s\n", error.first_str); > > + pr_debug("first_help : %s\n", error.first_help); > > + } > > + > > + evlist__delete(evlist); > > + free(error.str); > > + free(error.help); > > + free(error.first_str); > > + free(error.first_help); > > + return ret; > > +} > > This is quite similar to check_parse_id, fold them together? there is the 'same_cpu' logic in check_parse_id, so I did not want to mess with that > > > + > > +static int metric_parse_fake(const char *str) > > +{ > > + struct expr_parse_ctx ctx; > > + struct hashmap_entry *cur; > > + double result; > > + int ret = -1; > > + size_t bkt; > > + int i; > > + > > + pr_debug("parsing '%s'\n", str); > > + > > + expr__ctx_init(&ctx); > > + if (expr__find_other(str, NULL, &ctx, 0) < 0) { > > + pr_err("expr__find_other failed\n"); > > + return -1; > > + } > > + > > + i = 1; > > + hashmap__for_each_entry((&ctx.ids), cur, bkt) > > + expr__add_id(&ctx, strdup(cur->key), i++); > > It might make sense to share the code here with that in test_parsing. > This initialization of ids is commented there and it is a bit special. hum, not sure it's worth to add this complexity to test, I'd like to keep it simple, it's already not straightforward ;-) I added the comment you mentioned jirka