Re: [PATCH v2 04/17] libtraceeval histogram: Have cmp and release functions be generic

[Ross Zwisler <zwisler@google.com>]

On Fri, Aug 11, 2023 at 01:39:27AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Having the ability to override the compare function for a given type can
> be very advantageous. There's also no reason that any type could ask for a
> release callback to be called when the type is being released. It could be
> used for information as well as for freeing.
> 
> Rename traceeval_dyn_cmp_fn to traceeval_data_cmp_fn and
>  traceeval_dyn_release_fn to traceeval_data_release_fn and have
> them take the union traceeval_type instead of struct traceeval_dynamic.
> 
> Also this changes the compare to pass a pointer to union traceeval_data
> instead of passing in the structure of the dyn_data type.
> 
> In the structure, rename dyn_cmp to just cmp, and dyn_release to just
> release.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h | 46 +++++++++++++++++++++-------------------
>  src/histograms.c         | 28 ++++++++++++------------
>  2 files changed, 38 insertions(+), 36 deletions(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index f6c4e8efb2be..22e9029133d5 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -65,13 +65,13 @@ union traceeval_data {
>  struct traceeval_type;
>  
>  /* struct traceeval_dynamic release function signature */

You may want to update the comment here to make it clear that this is now for
both dynamic and pointer types.  Ditto for the compare function signature.

> -typedef void (*traceeval_dyn_release_fn)(struct traceeval_type *,
> -					 struct traceeval_dynamic);
> +typedef void (*traceeval_data_release_fn)(struct traceeval_type *,
> +					  union traceeval_data *);
>  
>  /* struct traceeval_dynamic compare function signature */
> -typedef int (*traceeval_dyn_cmp_fn)(struct traceeval_dynamic,
> -				    struct traceeval_dynamic,
> -				    struct traceeval_type *);
> +typedef int (*traceeval_data_cmp_fn)(const union traceeval_data *,
> +				     const union traceeval_data *,
> +				     struct traceeval_type *);
>  
>  /*
>   * struct traceeval_type - Describes the type of a traceevent_data instance
<>
> @@ -588,7 +588,7 @@ static int copy_traceeval_data(struct traceeval_type *type,
>  /*
>   * Free @data with respect to @size and @type.
>   *
> - * Does not call dyn_release on type TRACEEVAL_TYPE_DYNAMIC.
> + * Does not call release on type TRACEEVAL_TYPE_DYNAMIC.

or for TRACEEVAL_TYPE_POINTER.

>   */
>  static void data_release(size_t size, union traceeval_data **data,
>  				struct traceeval_type *type)
> -- 
> 2.40.1

Aside from these two comment nits, you can add:

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