Re: [PATCH 5/9] libtraceeval histogram: Add type to traceeval_data and make it a structure

[Ross Zwisler <zwisler@google.com>]

On Thu, Aug 17, 2023 at 06:24:18PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Having a straight union for passing in the data that must match the types
> is dangerous and prone for buggy code. If some data doesn't match its
> type, there's nothing to catch it.
> 
> Instead of having a union traceeval_data of each type, have it be a
> structure with a "type" field follow by a union, where the type defines
> what kind of data it is.
> 
> Add helper macros to make it easy to define the data when using it.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h |  88 ++++++++++++++++---------
>  samples/task-eval.c      | 137 ++++++++++++++++-----------------------
>  src/eval-local.h         |   4 +-
>  src/histograms.c         |  68 +++++++++----------
>  4 files changed, 150 insertions(+), 147 deletions(-)
> 
<>
> @@ -802,13 +779,9 @@ static void display_process_stats(struct traceeval *teval,
>  {
>  	struct traceeval_stat *stat;
>  	unsigned long long delta;
> -	union traceeval_data keys[] = {
> -		{
> -			.cstring = comm,
> -		},
> -		{
> -			.number = RUNNING,
> -		}
> +	struct traceeval_data keys[] = {
> +		DEFINE_TRACEEVAL_CSTRING(	comm		),
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
>  	};
>  
>  	for (int i = 0; i < OTHER; i++) {

Just after this in display_process_stats we have:

> 	for (int i = 0; i < OTHER; i++) {
> 		keys[1].number = i;

Which I think we want to set with the new macro TRACEEVAL_SET_NUMBER().

                TRACEEVAL_SET_NUMBER(keys[1], i)

Other than that you can add:

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