On Fri, Dec 07, 2012 at 05:38:22PM +0900, Namhyung Kim wrote: > Hi Arnaldo, >
SNIP > @@ -739,6 +739,10 @@ static struct hist_entry *hists__add_dummy_entry(struct > hists *hists, > > cmp = hist_entry__collapse(he, pair); > > + if (sort__need_collapse) > + cmp = hist_entry__collapse(he, pair); > + else > + cmp = hist_entry__cmp(pair, he); > if (!cmp) > goto out; > > @@ -772,7 +776,12 @@ static struct hist_entry *hists__find_entry(struct hists > *hists, > > while (n) { > struct hist_entry *iter = rb_entry(n, struct hist_entry, > rb_node_in); > - int64_t cmp = hist_entry__collapse(iter, he); > + int64_t cmp; > + > + if (sort__need_collapse) > + cmp = hist_entry__collapse(iter, he); > + else > + cmp = hist_entry__cmp(he, iter); > > if (cmp < 0) > n = n->rb_left; > > It doesn't look good, especially hist_entry__collapse will be same as > hist_entry__cmp if 'sort__need_collapse' is false. If we can make the > order consistent, it'd be converted to a sigle _collapse() call > without the conditional. > > Thanks, > Namhyung I've got non matching entries in diff after applying just patch 2/5, and it's caused by missing he/iter swap in initial name resort in insert_hist_entry_by_name function. I understand that function goes away in your next patch while add_hist_entry (swap ok) being the one doing initial name resort, but still.. took me some time to figure this out ;) jirka --- diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c index b2e7d39..4dda6f4 100644 --- a/tools/perf/builtin-diff.c +++ b/tools/perf/builtin-diff.c @@ -285,7 +285,7 @@ static void insert_hist_entry_by_name(struct rb_root *root, while (*p != NULL) { parent = *p; iter = rb_entry(parent, struct hist_entry, rb_node); - if (hist_entry__cmp(he, iter) < 0) + if (hist_entry__cmp(iter, he) < 0) p = &(*p)->rb_left; else p = &(*p)->rb_right; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/