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

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

[Roland Schwingel]

Hi Keith,

insight-owner@sourceware.org wrote on 27.03.2012 16:44:54:

 > In any case, please add a
 > ChangeLog entry for it, adjust the two formatting/CS discrepancies and
 > this patch is good to go in.
I corrected the formatting of function calls and indents and expanded
the changelog and checked it in now.

New changelog:
2012-03-28  Roland Schwingel <roland.schwingel@onevision.com>

    * generic/gdbtk.c (gdbtk_init,tk_command): Replace deprecated access
    to tcl interpreter result string with Tcl_GetStringResult().
    * generic/gdbtk-hooks.c (gdbtk_read,gdbtk_readline,gdbtk_load_hash)
    (gdbtk_query): Likewise.
    (gdbtk_read): Simplified error handling in case "gdbtk_console_read"
    fails. Some reformatting.

Thanks for reviewing. :-)
Do you know when you will find time to look at my other patches?
At least the next four of my bulk are quite trivial.

Thanks,

Roland

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

[Keith Seitz]

On 03/27/2012 12:13 AM, Roland Schwingel wrote:

> Sorry... Sometimes it is hard to switch between the styles. I surely
> want to follow the GCS when submitting patches. Our company style is
> just (in some areas) vice versa.

Not a problem -- it happens to everyone. :-)

> If you give the ok to my patch(es) I can also
> do these final reformatting during commit.

That will be best/easiest. One other thing I noticed:

 >        else
 > -        actual_len = strlen (gdbtk_interp->result);
 > +      {

Here ^

 > +       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;
 > +     }
 >      }

and here^ the identation level is off. Please adjust to follow the rest 
of the code. [These should be emacs-default indentations IIRC.]

> I did not change behaviour here Keith. gdbtk_read() behaves the same as
> before it just takes a small shortcut in case gdbtk_console_read has failed
> (for some reasons).

Yes, you are right. I completely missed that. In any case, please add a 
ChangeLog entry for it, adjust the two formatting/CS discrepancies and 
this patch is good to go in.

Thanks!
Keith

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

[Roland Schwingel]

Hi Keith...

Keith Seitz <keiths@redhat.com> wrote on 23.03.2012 23:25:06:
 > 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.
Sure no problem.

 > 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).
Sorry... Sometimes it is hard to switch between the styles. I surely
want to follow the GCS when submitting patches. Our company style is
just (in some areas) vice versa.

Shall I resubmit my patch(es)?
If you give the ok to my patch(es) I can also
do these final reformatting during commit.

 > > 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?
I did not change behaviour here Keith. gdbtk_read() behaves the same as
before it just takes a small shortcut in case gdbtk_console_read has failed
(for some reasons).

In the unpatched code actual_len is set to 0 in this case.
Execution continues and later on also results in
buf[actual_len] = '\0';
return actual_len;

where actual_len is 0. So same behaviour. I believe this was done
to bring the buffer in a defined (empty) state.

I did it that way to make the code easier to read as I am calling
Tcl_GetStringResult() and I just want to call it just once and
avoid additional ifs for handling the error case.

Maybe this was too obvious for me so I did not mention it in the
changelog and it does not change behaviour of the function.

Please tell me how to go on with the patches.
Resubmit (with -p flag on diff) and a second round on GCS?

Roland

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

[Keith Seitz]

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

[PATCH] Remove deprecated access to tcl internal variables

[Roland Schwingel]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

Hi...

Since tcl 8.6 access to some internal variables has been removed from 
the code. It was already deprecated since many years. Attached you find 
2 patches replacing the deprecated stuff with the current official way. 
This is also backward compatible to insight's tcl 8.4 version checkin in 
to sourceware.org.

Changelog:

2012-03-19  Roland Schwingel <roland.schwingel@onevision.com>

         * generic/gdbtk.c: (gdbtk_init,tk_command): Replace deprecated 
access
         to tcl interpreter result string with Tcl_GetStringResult().
         * generic/gdbtk-hooks.c (gdbtk_read,gdbtk_readline,gdbtk_load_hash)
         (gdbtk_query): Likewise

Any comments? Is this ok?

Roland

[-- Attachment #2: tcl_compat_gdbtk.c.patch --]
[-- Type: text/plain, Size: 1580 bytes --]

--- 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));
 
   /* Initialize the Paths variable.  */
   if (ide_initialize_paths (gdbtk_interp, "") != TCL_OK)
-    error ("ide_initialize_paths failed: %s", gdbtk_interp->result);
+    error ("ide_initialize_paths failed: %s", Tcl_GetStringResult(gdbtk_interp));
 
   if (Tk_Init (gdbtk_interp) != TCL_OK)
-    error ("Tk_Init failed: %s", gdbtk_interp->result);
+    error ("Tk_Init failed: %s", Tcl_GetStringResult(gdbtk_interp));
 
   if (Tktable_Init (gdbtk_interp) != TCL_OK)
-    error ("Tktable_Init failed: %s", gdbtk_interp->result);
+    error ("Tktable_Init failed: %s", Tcl_GetStringResult(gdbtk_interp));
 
   Tcl_StaticPackage (gdbtk_interp, "Tktable", Tktable_Init,
 		     (Tcl_PackageInitProc *) NULL);
@@ -560,7 +560,7 @@
 
   if (Gdbtk_Init (gdbtk_interp) != TCL_OK)
     {
-      error ("Gdbtk_Init failed: %s", gdbtk_interp->result);
+      error ("Gdbtk_Init failed: %s", Tcl_GetStringResult(gdbtk_interp));
     }
 
   Tcl_StaticPackage (gdbtk_interp, "Insight", Gdbtk_Init, NULL);
@@ -740,7 +740,7 @@
 
   retval = Tcl_Eval (gdbtk_interp, cmd);
 
-  result = xstrdup (gdbtk_interp->result);
+  result = xstrdup (Tcl_GetStringResult(gdbtk_interp));
 
   old_chain = make_cleanup (free, result);
 

[-- Attachment #3: tcl_compat_gdbtk-hooks.c.patch --]
[-- Type: text/plain, Size: 1494 bytes --]

--- 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;
 	}
       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
     {
@@ -507,11 +512,11 @@
 
   if (result == TCL_OK)
     {
-      return (xstrdup (gdbtk_interp->result));
+      return (xstrdup (Tcl_GetStringResult(gdbtk_interp)));
     }
   else
     {
-      gdbtk_fputs (gdbtk_interp->result, gdb_stdout);
+      gdbtk_fputs (Tcl_GetStringResult(gdbtk_interp), gdb_stdout);
       gdbtk_fputs ("\n", gdb_stdout);
       return (NULL);
     }
@@ -635,7 +640,7 @@
     report_error ();
   free(buf);
 
-  return atoi (gdbtk_interp->result);
+  return atoi (Tcl_GetStringResult(gdbtk_interp));
 }
 
 
@@ -689,7 +694,7 @@
   gdbtk_two_elem_cmd ("gdbtk_tcl_query", buf);
   free(buf);
 
-  val = atol (gdbtk_interp->result);
+  val = atol (Tcl_GetStringResult(gdbtk_interp));
   return val;
 }