| Message ID | 20230302171755.1821653-2-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | tracing/hist: add a modulus operator | expand |
On Thu, 2 Mar 2023 17:17:54 +0000 Mark Rutland <mark.rutland@arm.com> 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@arm.com> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Steven Rostedt (Google) <rostedt@goodmis.org> > Cc: Tom Zanussi <zanussi@kernel.org> > Cc: linux-trace-kernel@vger.kernel.org > --- > 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; > } >
On Sat, Mar 18, 2023 at 03:12:08PM -0400, Steven Rostedt wrote: > On Thu, 2 Mar 2023 17:17:54 +0000 > Mark Rutland <mark.rutland@arm.com> 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() > Sorry; I always get this wrong since many other trees do everything lower case (or support total commit message anarchy). I'll go fix that up. > > > 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. Sure. > > > > > 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@arm.com> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: Steven Rostedt (Google) <rostedt@goodmis.org> > > Cc: Tom Zanussi <zanussi@kernel.org> > > Cc: linux-trace-kernel@vger.kernel.org > > --- > > 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! Ugh, sorry, I had completely glossed over the: if (sep) { ... // assignments to *sep here ... } ... in the existing code. I'll go rework that... > > > > > /* > > - * 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? As in the commit message, that was just so that every check for an operator had the same shape. I can certainly drop this for the first check and just have: op = strrchr(str, '-'); if (op) { ... } > > 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? Yep, I got this wrong. I'll go rejig that. Thanks, Mark.
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; /* - * 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) { + *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; } - plus_op = strrchr(str, '+'); - if (plus_op || minus_op) { - /* - * For operators of the same precedence use to rightmost as the - * root, so that the expression is evaluated left to right. - */ - if (plus_op > minus_op) - field_op = FIELD_OP_PLUS; - goto out; + op = strrchr(str, '+'); + if (op > *sep) { + *sep = op; + field_op = FIELD_OP_PLUS; } /* - * Multiplication and division have higher precedence than addition and - * subtraction. + * If we've found a low-precedence operator, we're done. */ - div_op = strrchr(str, '/'); - if (div_op) - field_op = FIELD_OP_DIV; + if (*sep) + return field_op; - mult_op = strrchr(str, '*'); /* - * For operators of the same precedence use to rightmost as the - * root, so that the expression is evaluated left to right. + * Second, consider the higher precedence multiplication and division + * operators. */ - if (mult_op > div_op) - field_op = FIELD_OP_MULT; + op = strrchr(str, '/'); + if (op > *sep) { + *sep = op; + field_op = FIELD_OP_DIV; + } -out: - if (sep) { - switch (field_op) { - case FIELD_OP_UNARY_MINUS: - case FIELD_OP_MINUS: - *sep = minus_op; - break; - case FIELD_OP_PLUS: - *sep = plus_op; - break; - case FIELD_OP_DIV: - *sep = div_op; - break; - case FIELD_OP_MULT: - *sep = mult_op; - break; - case FIELD_OP_NONE: - default: - *sep = NULL; - break; - } + op = strrchr(str, '*'); + if (op > *sep) { + *sep = op; + field_op = FIELD_OP_MULT; } return field_op;
In a subsequent patch we'll add 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@arm.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tom Zanussi <zanussi@kernel.org> Cc: linux-trace-kernel@vger.kernel.org --- kernel/trace/trace_events_hist.c | 80 ++++++++++++-------------------- 1 file changed, 30 insertions(+), 50 deletions(-)