Re: [PATCH v5 1/4] perf util: Move block_pair_cmp to block-info

[Arnaldo Carvalho de Melo]

Em Tue, Jan 28, 2020 at 08:55:53PM +0800, Jin Yao escreveu:
> block_pair_cmp() is a function which is used to compare
> two blocks. Moving it from builtin-diff.c to block-info.c
> to let it can be used by other builtins.
> 
>  v4/v5:
>  ------
>  No change.
> 
>  v3:
>  ---
>  Separate it from original patch for good tracking.
> 
> Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> ---
>  tools/perf/builtin-diff.c    | 17 -----------------
>  tools/perf/util/block-info.c | 17 +++++++++++++++++
>  tools/perf/util/block-info.h |  2 ++
>  3 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index f8b6ae557d8b..5ff1e21082cb 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -572,23 +572,6 @@ static void init_block_hist(struct block_hist *bh)
>  	bh->valid = true;
>  }
>  
> -static int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
> -{
> -	struct block_info *bi_a = a->block_info;
> -	struct block_info *bi_b = b->block_info;
> -	int cmp;
> -
> -	if (!bi_a->sym || !bi_b->sym)
> -		return -1;
> -
> -	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
> -
> -	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
> -		return 0;
> -
> -	return -1;
> -}
> -
>  static struct hist_entry *get_block_pair(struct hist_entry *he,
>  					 struct hists *hists_pair)
>  {
> diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
> index c4b030bf6ec2..f0f38bdd496a 100644
> --- a/tools/perf/util/block-info.c
> +++ b/tools/perf/util/block-info.c
> @@ -475,3 +475,20 @@ float block_info__total_cycles_percent(struct hist_entry *he)
>  
>  	return 0.0;
>  }
> +
> +int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)

First thing that came to mind was that hist_entry comparision functions
had been changed to return int64_t recently, when I went to look at it I
found this:

tools/perf/util/block-info.c

int64_t block_info__cmp(struct perf_hpp_fmt *fmt __maybe_unused,
                        struct hist_entry *left, struct hist_entry *right)
{
        struct block_info *bi_l = left->block_info;
        struct block_info *bi_r = right->block_info;
        int cmp;
.
.
.

Which look a bit more complete, can you check if that can be used
instead or explain why my quick analysis of this is b0rken?

Perhaps we can have a __block_info__cmp() that doesn't receive the
perf_hpp_fmt (that isn't even used above) so that the previous use of
block_pair_cmp() can be replaced with __block_info__cmp() instead?

Thanks,

- Arnaldo

> +{
> +	struct block_info *bi_a = a->block_info;
> +	struct block_info *bi_b = b->block_info;
> +	int cmp;
> +
> +	if (!bi_a->sym || !bi_b->sym)
> +		return -1;
> +
> +	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
> +
> +	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
> +		return 0;
> +
> +	return -1;
> +}
> diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
> index bef0d75e9819..4fa91eeae92e 100644
> --- a/tools/perf/util/block-info.h
> +++ b/tools/perf/util/block-info.h
> @@ -76,4 +76,6 @@ int report__browse_block_hists(struct block_hist *bh, float min_percent,
>  
>  float block_info__total_cycles_percent(struct hist_entry *he);
>  
> +int block_pair_cmp(struct hist_entry *a, struct hist_entry *b);
> +
>  #endif /* __PERF_BLOCK_H */
> -- 
> 2.17.1
> 

-- 

- Arnaldo