Re: [PATCH v2 05/17] libtraceeval histograms: Add traceeval struct to compare function

[Ross Zwisler <zwisler@google.com>]

On Fri, Aug 11, 2023 at 01:39:28AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> When looking at how this code would be implemented, I found that I needed
> access to the traceeval structure within the compare callbacks. Pass that
> in too.
> 
> Also, rearrange the parameters a little. The traceeval and type should go
> first as they describe the "object", and the data should go last as they are
> the values of the function.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h | 13 +++++++------
>  src/histograms.c         | 19 ++++++++++---------
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 22e9029133d5..412efdbe8681 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -63,16 +63,17 @@ union traceeval_data {
>  };
>  
>  struct traceeval_type;
> +struct traceeval;
>  
>  /* struct traceeval_dynamic release function signature */
> -typedef void (*traceeval_data_release_fn)(struct traceeval_type *,
> -					  union traceeval_data *);
> +typedef void (*traceeval_data_release_fn)(const struct traceeval_type *type,
> +					  union traceeval_data *data);
>  
>  /* struct traceeval_dynamic compare function signature */
> -typedef int (*traceeval_data_cmp_fn)(const union traceeval_data *,
> -				     const union traceeval_data *,
> -				     struct traceeval_type *);
> -
> +typedef int (*traceeval_data_cmp_fn)(struct traceeval *teval,
> +				     const struct traceeval_type *type,
> +				     const union traceeval_data *A,
> +				     const union traceeval_data *B);
>  /*
>   * struct traceeval_type - Describes the type of a traceevent_data instance
>   * @type: The enum type that describes the traceeval_data
> diff --git a/src/histograms.c b/src/histograms.c
> index 09cdf57a8f6a..ece1395ac300 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -113,7 +113,8 @@ static int compare_traceeval_type(struct traceeval_type *orig,
>   * Return 0 if @orig and @copy are the same, 1 if @orig is greater than @copy,
>   * -1 for the other way around, and -2 on error.
>   */
> -static int compare_traceeval_data(union traceeval_data *orig,
> +static int compare_traceeval_data(struct traceeval *teval,
> +				  union traceeval_data *orig,
>  				  const union traceeval_data *copy,
>  				  struct traceeval_type *type)

Nit: it would be nice if these had the same ordering as the args to
traceeval_data_cmp_fn.

Other than that you can add:

Reviewed-by: Ross Zwisler <zwisler@google.com>