Re: [PATCH 14/14] perf diff: Add generic order option for compute sorting

Jiri Olsa
2012-11-29T12:20:59Z

On Thu, Nov 29, 2012 at 09:02:01PM +0900, Namhyung Kim wrote:
> On Wed, 28 Nov 2012 14:52:49 +0100, Jiri Olsa wrote:
> > Adding option 'o' to allow sorting based on the
> > input file number.
> [snip]
> > hist_entry__cmp_compute(struct hist_entry *left, struct hist_entry *right,
> > int c)
> > {
> > - int i;
> > + struct hist_entry **pairs_left = left->pairs;
> > + struct hist_entry **pairs_right = right->pairs;
> > + struct hist_entry *p_right, *p_left;
> > + static int64_t cmp;
> >
> > - for (i = 0; i < data_cnt; i++) {
> > - struct hist_entry **pairs_left = left->pairs;
> > - struct hist_entry **pairs_right = right->pairs;
> > - struct hist_entry *p_right, *p_left;
> > - static int64_t cmp;
> > + if (!pairs_left || !pairs_right)
> > + return pairs_left ? -1 : 1;
> >
> > - if (!pairs_left || !pairs_right)
> > - return pairs_right - pairs_left;
> > + p_right = pairs_right[sort_compute];
> > + p_left = pairs_left[sort_compute];
> >
> > - p_right = pairs_right[i];
> > - p_left = pairs_left[i];
> > + if (!p_left || !p_right)
> > + return p_left ? -1 : 1;
>
> What if both p_left and p_right are NULL? Shouldn't it be move to the
> next pairs?
hm, right.. we bail out, but not sure what to return.. 0,-1 or 1

>
> >
> > - if (!p_left || !p_right)
> > - return p_right - p_left;
> > -
> > - /*
> > - * If we differ, we are done, otherwise continue until all
> > - * is processed or we find a difference.
> > - */
> > - cmp = __hist_entry__cmp_compute(p_left, p_right, c);
> > - if (cmp)
> > - return cmp;
> > - }
> > + /*
> > + * If we differ, we are done, otherwise continue until all
> > + * is processed or we find a difference.
> > + */
>
> I guess this comment is not applied anymore. Or we need a loop after
> checking sort_compute column, right?

unfortunatelly formated.. if we get this far, we have numbers to compare..
meaning: we have 2 lines that have same, non empty column to compare

thanks,
jirka