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

[Roland Schwingel <roland at onevision dot 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