From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26882 invoked by alias); 24 Aug 2011 09:30:49 -0000 Received: (qmail 26870 invoked by uid 22791); 24 Aug 2011 09:30:47 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-fx0-f41.google.com (HELO mail-fx0-f41.google.com) (209.85.161.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 24 Aug 2011 09:30:21 +0000 Received: by fxg9 with SMTP id 9so1006613fxg.0 for ; Wed, 24 Aug 2011 02:30:20 -0700 (PDT) Received: by 10.223.4.211 with SMTP id 19mr5291839fas.95.1314178220179; Wed, 24 Aug 2011 02:30:20 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.120.207 with HTTP; Wed, 24 Aug 2011 02:29:40 -0700 (PDT) In-Reply-To: <4E54369C.9000706@earthlink.net> References: <4E406AE9.100@earthlink.net> <4E54369C.9000706@earthlink.net> From: Hui Zhu Date: Wed, 24 Aug 2011 09:30:00 -0000 Message-ID: Subject: Re: [PATCH]tracepoint.c: Add conditionals num to find_matching_tracepoint To: Stan Shebs Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00432.txt.bz2 On Wed, Aug 24, 2011 at 07:24, Stan Shebs wrote: > 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 =A0Hui Zhu >> >> =A0 =A0 =A0 =A0* tracepoint.c (cond_string_is_same): New function. >> =A0 =A0 =A0 =A0(find_matching_tracepoint): Add condition check >> =A0 =A0 =A0 =A0by cond_string_is_same. >> --- >> =A0tracepoint.c | =A0 19 ++++++++++++++++++- >> =A01 file changed, 18 insertions(+), 1 deletion(-) >> >> --- a/tracepoint.c >> +++ b/tracepoint.c >> @@ -3091,6 +3091,22 @@ free_uploaded_tsvs (struct uploaded_tsv >> =A0 =A0 =A0} >> =A0} >> >> +static int >> +cond_string_is_same(char *str1, char *str2) > > Don't forget the all-important space! :-) Sorry for forgot it. > >> +{ >> + =A0if (str1 =3D=3D NULL || str2 =3D=3D NULL) >> + =A0 =A0{ >> + =A0 =A0 =A0if (str1 =3D=3D str2) >> + =A0 =A0 =A0 return 1; >> + =A0 =A0 =A0else >> + =A0 =A0 =A0 return 0; > > This bit is a little simpler as just "return (str1 =3D=3D str2);" My code is so ugly. Thanks for help me make it more better. :) > >> + =A0 =A0} >> + =A0if (strcmp (str1, str2)) >> + =A0 =A0return 0; >> + >> + =A0return 1; > > Typically, I would write this as "return (strcmp (str1, str2) =3D=3D 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 =3D=3D 0" vs "x=3D=3D0" -= but that's > generally OK, because it just results in an extra tracepoint that is easi= ly > deleted. That is really a issue for us. I add a comment for it. Wish I can get some time to make it better. > > OK to install with these changes, and thanks! I made a new patch according to your patch and checked it in. Thanks for your help. Best, Hui > > Stan > stan@codesourcery.com > >> +} >> + >> =A0/* Look for an existing tracepoint that seems similar enough to the >> =A0 =A0 uploaded one. =A0Enablement isn't compared, because the user can >> =A0 =A0 toggle that freely, and may have done so in anticipation of the >> @@ -3111,7 +3127,8 @@ find_matching_tracepoint (struct uploade >> =A0 =A0 =A0 =A0if (b->type =3D=3D utp->type >> =A0 =A0 =A0 =A0&& =A0t->step_count =3D=3D utp->step >> =A0 =A0 =A0 =A0&& =A0t->pass_count =3D=3D utp->pass >> - =A0 =A0 =A0 =A0 /* FIXME also test conditionals and actions. =A0*/ >> + =A0 =A0 =A0 && =A0cond_string_is_same (t->base.cond_string, utp->cond_= string) >> + =A0 =A0 =A0 =A0 /* FIXME also test actions. =A0*/ >> =A0 =A0 =A0 =A0 =A0) >> =A0 =A0 =A0 =A0{ >> =A0 =A0 =A0 =A0 =A0/* Scan the locations for an address match. =A0*/ >> > > http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/tracepoint.c.diff?cvsroot= =3Dsrc&r1=3D1.230&r2=3D1.231 2011-08-24 Hui Zhu * tracepoint.c (cond_string_is_same): New function. (find_matching_tracepoint): Add condition check by cond_string_is_same. =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /cvs/src/src/gdb/tracepoint.c,v retrieving revision 1.230 retrieving revision 1.231 diff -u -r1.230 -r1.231 --- src/gdb/tracepoint.c 2011/08/04 19:10:13 1.230 +++ src/gdb/tracepoint.c 2011/08/24 09:24:10 1.231 @@ -3091,6 +3091,19 @@ } } +/* FIXME this function is heuristic and will miss the cases where the + conditional is semantically identical but differs in whitespace, + such as "x =3D=3D 0" vs "x=3D=3D0". */ + +static int +cond_string_is_same (char *str1, char *str2) +{ + if (str1 =3D=3D NULL || str2 =3D=3D NULL) + return (str1 =3D=3D str2); + + return (strcmp (str1, str2) =3D=3D 0); +} + /* 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 +3124,8 @@ if (b->type =3D=3D utp->type && t->step_count =3D=3D utp->step && t->pass_count =3D=3D 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. */