Re: [PATCH]tracepoint.c: Add conditionals num to find_matching_tracepoint

Stan Shebs
2011-08-23T23:24:45Z

On 8/14/11 6:40 AM, Hui Zhu wrote:
> [...]
> Hi Stan,
>
> Thanks for your review.
>
> I make a new patch that check the condition according to your mail.
>
> Best,
> Hui
>
> 2011-08-14 Hui Zhu<teawater@gmail.com>
>
> * tracepoint.c (cond_string_is_same): New function.
> (find_matching_tracepoint): Add condition check
> by cond_string_is_same.
> ---
> tracepoint.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> --- a/tracepoint.c
> +++ b/tracepoint.c
> @@ -3091,6 +3091,22 @@ free_uploaded_tsvs (struct uploaded_tsv
> }
> }
>
> +static int
> +cond_string_is_same(char *str1, char *str2)

Don't forget the all-important space! :-)

> +{
> + if (str1 == NULL || str2 == NULL)
> + {
> + if (str1 == str2)
> + return 1;
> + else
> + return 0;

This bit is a little simpler as just "return (str1 == str2);"

> + }
> + if (strcmp (str1, str2))
> + return 0;
> +
> + return 1;

Typically, I would write this as "return (strcmp (str1, str2) == 0);"

It would also be good to add a function header comment that this
function is heuristic and will miss the cases where the conditional is
semantically identical but differs in whitespace, such as "x == 0" vs
"x==0" - but that's generally OK, because it just results in an extra
tracepoint that is easily deleted.

OK to install with these changes, and thanks!

Stan
stan@codesourcery.com

> +}
> +
> /* Look for an existing tracepoint that seems similar enough to the
> uploaded one. Enablement isn't compared, because the user can
> toggle that freely, and may have done so in anticipation of the
> @@ -3111,7 +3127,8 @@ find_matching_tracepoint (struct uploade
> if (b->type == utp->type
> && t->step_count == utp->step
> && t->pass_count == utp->pass
> - /* FIXME also test conditionals and actions. */
> + && cond_string_is_same (t->base.cond_string, utp->cond_string)
> + /* FIXME also test actions. */
> )
> {
> /* Scan the locations for an address match. */
>