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

[Roland Schwingel <roland@onevision.com>]

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