* Re: [PATCH] Remove deprecated access to tcl internal variables
@ 2012-03-28 14:53 Roland Schwingel
0 siblings, 0 replies; 5+ messages in thread
From: Roland Schwingel @ 2012-03-28 14:53 UTC (permalink / raw)
To: insight
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Remove deprecated access to tcl internal variables
@ 2012-03-27 7:13 Roland Schwingel
2012-03-27 15:18 ` Keith Seitz
0 siblings, 1 reply; 5+ messages in thread
From: Roland Schwingel @ 2012-03-27 7:13 UTC (permalink / raw)
To: insight, keiths
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Remove deprecated access to tcl internal variables
2012-03-27 7:13 Roland Schwingel
@ 2012-03-27 15:18 ` Keith Seitz
0 siblings, 0 replies; 5+ messages in thread
From: Keith Seitz @ 2012-03-27 15:18 UTC (permalink / raw)
To: Roland Schwingel; +Cc: insight
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Remove deprecated access to tcl internal variables
@ 2012-03-19 11:46 Roland Schwingel
2012-03-23 22:25 ` Keith Seitz
0 siblings, 1 reply; 5+ messages in thread
From: Roland Schwingel @ 2012-03-19 11:46 UTC (permalink / raw)
To: insight
[-- 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;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Remove deprecated access to tcl internal variables
2012-03-19 11:46 Roland Schwingel
@ 2012-03-23 22:25 ` Keith Seitz
0 siblings, 0 replies; 5+ messages in thread
From: Keith Seitz @ 2012-03-23 22:25 UTC (permalink / raw)
To: Roland Schwingel; +Cc: insight
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-03-28 14:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-28 14:53 [PATCH] Remove deprecated access to tcl internal variables Roland Schwingel
-- strict thread matches above, loose matches on Subject: below --
2012-03-27 7:13 Roland Schwingel
2012-03-27 15:18 ` Keith Seitz
2012-03-19 11:46 Roland Schwingel
2012-03-23 22:25 ` Keith Seitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).