Re: [PATCH 1/2] tracing/hist: simplify contains_operator()

[Steven Rostedt]

On Thu,  2 Mar 2023 17:17:54 +0000
Mark Rutland <mark.rutland@xxxxxxx> wrote:

FYI, we follow Linus's preference that subjects start with a capital
letter. Unless of course you are a socialist and dislike capitalism?

  tracing/hist: Simplify contains_operator()


> In a subsequent patch we'll add additional operators for histogram
> expressions.

Refrain from using "subsequent patch", instead say:

 Simplify contains_operator() in order to support additional operators
 for histogram expressions.

> 
> In preparation for adding additional operators, this patch refactors
> contains_operator() to consider each operator within a precedence group
> independently by using the 'sep' pointer as the current rightmost
> operator, and removing the separate op pointers.
> 
> Within each precedence group, this allows operators to be checked
> independently with a consistent pattern:
> 
> 	op = strrchr(str, $OP_CHAR);
> 	if (op > *sep) {
> 		*sep = op;
> 		field_op = $FIELD_OP_TYPE;
> 	}
> 
> This makes it easy to add new operators of the same precedence without
> needing to check multiple pointers, and without needing a final switch
> statement to recover the relevant pointer.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Cc: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> Cc: Tom Zanussi <zanussi@xxxxxxxxxx>
> Cc: linux-trace-kernel@xxxxxxxxxxxxxxx
> ---
>  kernel/trace/trace_events_hist.c | 80 ++++++++++++--------------------
>  1 file changed, 30 insertions(+), 50 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 10d36f751fcd..a308da2cde2f 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1813,13 +1813,15 @@ static char *expr_str(struct hist_field *field, unsigned int level)
>  static int contains_operator(char *str, char **sep)
>  {
>  	enum field_op_id field_op = FIELD_OP_NONE;
> -	char *minus_op, *plus_op, *div_op, *mult_op;
> +	char *op;
>  
> +	*sep = NULL;

Hmm!

>  
>  	/*
> -	 * Report the last occurrence of the operators first, so that the
> -	 * expression is evaluated left to right. This is important since
> -	 * subtraction and division are not associative.
> +	 * For operators of the same precedence report the last occurrence of
> +	 * the operators first, so that the expression is evaluated left to
> +	 * right. This is important since subtraction and division are not
> +	 * associative.
>  	 *
>  	 *	e.g
>  	 *		64/8/4/2 is 1, i.e 64/8/4/2 = ((64/8)/4)/2
> @@ -1830,68 +1832,46 @@ static int contains_operator(char *str, char **sep)
>  	 * First, find lower precedence addition and subtraction
>  	 * since the expression will be evaluated recursively.
>  	 */
> -	minus_op = strrchr(str, '-');
> -	if (minus_op) {
> +	op = strrchr(str, '-');
> +	if (op > *sep) {

Why compare to *sep if it is always NULL?

Oh! But later in the code we have:

	if (contains_operator(field, NULL) || is_var_ref(field))

I wonder how *sep = NULL will handle that?

-- Steve

> +		*sep = op;
> +
>  		/*
>  		 * Unary minus is not supported in sub-expressions. If
>  		 * present, it is always the next root operator.
>  		 */
> -		if (minus_op == str) {
> -			field_op = FIELD_OP_UNARY_MINUS;
> -			goto out;
> -		}
> +		if (op == str)
> +			return FIELD_OP_UNARY_MINUS;
>  
>  		field_op = FIELD_OP_MINUS;
>  	}
>