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

[Stan Shebs <stanshebs@earthlink.net>]

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