Re: [PATCH v7 1/3] perf report: Change sort order by a specified event in group

[Arnaldo Carvalho de Melo]

Em Thu, Feb 20, 2020 at 09:36:14AM +0800, Jin Yao escreveu:
> When performing "perf report --group", it shows the event group information
> together. By default, the output is sorted by the first event in group.
> 
> It would be nice for user to select any event for sorting. This patch
> introduces a new option "--group-sort-idx" to sort the output by the
> event at the index n in event group.
> 
> For example,
> 
> Before:

Tested and applied, I also did the following simplifications, to try and
follow naming conventions and use less lines to do the same thing:

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 35224b366305..025f4c7f96bf 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -151,44 +151,35 @@ static int field_cmp(u64 field_a, u64 field_b)
 	return 0;
 }
 
-static int pair_fields_alloc(struct hist_entry *a, struct hist_entry *b,
-			     hpp_field_fn get_field, int nr_members,
-			     u64 **fields_a, u64 **fields_b)
+static int hist_entry__new_pair(struct hist_entry *a, struct hist_entry *b,
+				hpp_field_fn get_field, int nr_members,
+				u64 **fields_a, u64 **fields_b)
 {
-	struct evsel *evsel;
+	u64 *fa = calloc(nr_members, sizeof(*fa)),
+	    *fb = calloc(nr_members, sizeof(*fb));
 	struct hist_entry *pair;
-	u64 *fa, *fb;
-	int ret = -1;
-
-	fa = calloc(nr_members, sizeof(*fa));
-	fb = calloc(nr_members, sizeof(*fb));
 
 	if (!fa || !fb)
-		goto out;
+		goto out_free;
 
 	list_for_each_entry(pair, &a->pairs.head, pairs.node) {
-		evsel = hists_to_evsel(pair->hists);
+		struct evsel *evsel = hists_to_evsel(pair->hists);
 		fa[perf_evsel__group_idx(evsel)] = get_field(pair);
 	}
 
 	list_for_each_entry(pair, &b->pairs.head, pairs.node) {
-		evsel = hists_to_evsel(pair->hists);
+		struct evsel *evsel = hists_to_evsel(pair->hists);
 		fb[perf_evsel__group_idx(evsel)] = get_field(pair);
 	}
 
 	*fields_a = fa;
 	*fields_b = fb;
-	ret = 0;
-
-out:
-	if (ret != 0) {
-		free(fa);
-		free(fb);
-		*fields_a = NULL;
-		*fields_b = NULL;
-	}
-
-	return ret;
+	return 0;
+out_free:
+	free(fa);
+	free(fb);
+	*fields_a = *fields_b = NULL;
+	return -1;
 }
 
 static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
@@ -206,8 +197,7 @@ static int __hpp__group_sort_idx(struct hist_entry *a, struct hist_entry *b,
 	if (idx < 1 || idx >= nr_members)
 		return cmp;
 
-	ret = pair_fields_alloc(a, b, get_field, nr_members,
-				&fields_a, &fields_b);
+	ret = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
 	if (ret) {
 		ret = cmp;
 		goto out;
@@ -254,8 +244,7 @@ static int __hpp__sort(struct hist_entry *a, struct hist_entry *b,
 		return ret;
 
 	nr_members = evsel->core.nr_members;
-	i = pair_fields_alloc(a, b, get_field, nr_members,
-			      &fields_a, &fields_b);
+	i = hist_entry__new_pair(a, b, get_field, nr_members, &fields_a, &fields_b);
 	if (i)
 		goto out;