Re: [PATCH] Remove deprecated access to tcl internal variables

[Keith Seitz <keiths@redhat.com>]

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