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

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 15 Aug 2023 10:50:28 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > 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.

Agreed, (I was tired and new I was going to miss some spots).

> 
> > -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.

+1

> 
> >   */
> >  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>

Thanks Ross!

-- Steve