Re: [PATCH 4/5] histograms: traceeval compare

[Ross Zwisler <zwisler@google.com>]

On Fri, Jul 28, 2023 at 03:04:39PM -0400, Stevie Alvarez wrote:
> From: "Stevie Alvarez (Google)" <stevie.6strings@gmail.com>
> 
> traceeval_compare() compares two struct traceeval instances for
> equality. This suite of comparitors was made for testing purposes.
> 
> Signed-off-by: Stevie Alvarez (Google) <stevie.6strings@gmail.com>
> ---
>  src/histograms.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 176 insertions(+), 2 deletions(-)
> 
> diff --git a/src/histograms.c b/src/histograms.c
> index f46a0e0..5f1c7ef 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -20,6 +20,19 @@
>  #define for_each_key(i, keys)	\
>  	for (i = 0; (keys)[(i)].type != TRACEEVAL_TYPE_NONE; (i)++)
>  
> +/**
> + * Compare two integers of variable length.
> + *
> + * Return 0 if @a and @b are the same, 1 if @a is greater than @b, and -1
> + * if @b is greater than @a.
> + */
> +#define compare_numbers_return(a, b)	\
> +do {					\
> +	if ((a) < (b))			\
> +		return -1;		\
> +	return (a) != (b);		\
> +} while (0)				\
> +
>  /** A key-value pair */
>  struct entry {
>  	union traceeval_data	*keys;
> @@ -56,10 +69,171 @@ static void print_err(const char *fmt, ...)
>  	fprintf(stderr, "\n");
>  }
>  
> -// TODO
> +/**
> + * Return 0 if @orig and @copy are the same, 1 otherwise.
> + */
> +static int compare_traceeval_type(struct traceeval_type *orig,
> +		struct traceeval_type *copy)
> +{
> +	int o_name_null;
> +	int c_name_null;
> +
> +	// same memory/null
> +	if (orig == copy)
> +		return 0;
> +
> +	size_t i = 0;

Best practice is to put all the variable definitions at the top of the
function.

> +	do {
> +		if (orig[i].type != copy[i].type)
> +			return 1;
> +		if (orig[i].type == TRACEEVAL_TYPE_NONE)
> +			return 0;
> +		if (orig[i].flags != copy[i].flags)
> +			return 1;
> +		if (orig[i].id != copy[i].id)
> +			return 1;
> +		if (orig[i].dyn_release != copy[i].dyn_release)
> +			return 1;
> +		if (orig[i].dyn_cmp != copy[i].dyn_cmp)
> +			return 1;
> +
> +		// make sure both names are same type
> +		o_name_null = !orig[i].name;
> +		c_name_null = !copy[i].name;
> +		if (o_name_null != c_name_null)
> +			return 1;
> +		if (o_name_null)
> +			continue;
> +		if (strcmp(orig[i].name, copy[i].name) != 0)
> +			return 1;
> +	} while (orig[i++].type != TRACEEVAL_TYPE_NONE);
> +
> +	return 0;
> +}

<snip>

> +/**
> + * Return 0 if struct hist_table of @orig and @copy are the same, 1 if not,
> + * and -1 on error.
> + */
> +static int compare_hist(struct traceeval *orig, struct traceeval *copy)
> +{
> +	struct hist_table *o_hist = orig->hist;
> +	struct hist_table *c_hist = copy->hist;
> +	int cnt = !(o_hist->nr_entries == c_hist->nr_entries);
> +	if (cnt)
> +		return 1;
> +
> +	for (size_t i = 0; i < o_hist->nr_entries; i++) {
> +		// cmp each entry
> +		compare_entries(&o_hist->map[i], &c_hist->map[i], orig);

Need to check the return value of 'compare_entries()' and return it if it's
nonzero.

> +	}
> +	return 0;	
> +}
> +
> +/**
> + * Return 0 if @orig and @copy are the same, 1 if not, -1 if error.
> + */
>  int traceeval_compare(struct traceeval *orig, struct traceeval *copy)
>  {
> -	return -1;
> +	if ((!orig) || (!copy))

No need for parens, this can just be:

  if (!orig || !copy)
    return -1;

> +		return -1;
> +
> +	int keys = compare_traceeval_type(orig->def_keys, copy->def_keys);
> +	int vals = compare_traceeval_type(orig->def_vals, copy->def_vals);
> +	int hists = compare_hist(orig, copy);
> +
> +	return (keys || vals || hists);
>  }
>  
>  /**
> -- 
> 2.41.0
>