From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10866 invoked by alias); 23 Mar 2012 22:25:44 -0000 Received: (qmail 10838 invoked by uid 22791); 23 Mar 2012 22:25:43 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BT,TW_CP,TW_DB,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 23 Mar 2012 22:25:22 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q2NMPANC002722 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 23 Mar 2012 18:25:10 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q2NMP7G9001798 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 23 Mar 2012 18:25:09 -0400 Message-ID: <4F6CF842.7030400@redhat.com> Date: Fri, 23 Mar 2012 22:25:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120209 Thunderbird/10.0.1 MIME-Version: 1.0 To: Roland Schwingel CC: insight@sourceware.org Subject: Re: [PATCH] Remove deprecated access to tcl internal variables References: <4F671C73.2020308@onevision.com> In-Reply-To: <4F671C73.2020308@onevision.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact insight-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: insight-owner@sourceware.org X-SW-Source: 2012-q1/txt/msg00036.txt.bz2 I noticed this a while back, too.. Some small nits. In the future, could you please add "-p" option to diff? I have "diff -upN" set in my .cvsrc. It gives me a little more context when reading the patch. On 03/19/2012 04:45 AM, Roland Schwingel wrote: > --- gdbtk_orig/generic/gdbtk.c 2012-03-19 11:21:15.542232400 +0100 > +++ gdbtk/generic/gdbtk.c 2012-03-19 11:23:12.099170600 +0100 > @@ -494,17 +494,17 @@ > make_final_cleanup (gdbtk_cleanup, NULL); > > if (Tcl_Init (gdbtk_interp) != TCL_OK) > - error ("Tcl_Init failed: %s", gdbtk_interp->result); > + error ("Tcl_Init failed: %s", Tcl_GetStringResult(gdbtk_interp)); Watch the spaces in between function names and open parenthesis. This is a GNU convention (which we follow). That should be "Tcl_GetStringResult (gdbtk_interp)". There are several places where this occurs throughout the patches (and subsequent ones). > tcl_compat_gdbtk-hooks.c.patch > > > --- gdbtk_orig/generic/gdbtk-hooks.c 2012-01-03 13:26:56.000000000 +0100 > +++ gdbtk/generic/gdbtk-hooks.c 2012-03-05 11:47:03.340565000 +0100 > @@ -254,17 +254,22 @@ > { > report_error (); > actual_len = 0; > + buf[0] = '\0'; > + return 0; This hunk is not mentioned in the ChangeLog. I don't understand this. If we return 0 here, that means that no bytes in the buffer were filled-in, yet we are actually filling in one byte anyway. Is there a problem this was designed to solve? Is the return value not being checked somewhere or buf assumed to have at least the string terminal in it? > } > else > - actual_len = strlen (gdbtk_interp->result); > + { > + const char *tclResult = Tcl_GetStringResult(gdbtk_interp); > + actual_len = strlen (tclResult); > > /* Truncate the string if it is too big for the caller's buffer. */ > if (actual_len>= sizeof_buf) > actual_len = sizeof_buf - 1; > > - memcpy (buf, gdbtk_interp->result, actual_len); > + memcpy (buf,tclResult, actual_len); > buf[actual_len] = '\0'; > return actual_len; > + } > } > else > { Keith