Re: Patch: fixlet in gdbtk-cmds.c

[Syd Polk <spolk@redhat.com>]

This all seems quite wacky. I don't know enough about the code to
comment, but it smells really bad.

I would rather have a global variable for whether a gdbtk command
generated the error as opposed to Tcl itself rather than messing with
interp result flags. This is just asking to break when the interp data
structure is changed.

I will examine this more in a couple of weeks.

Tom Tromey wrote:
> 
> Syd> Approved, although I would have preferred using
> Syd> Tcl_WrongNumArgs. That would give a slightly different error
> Syd> message, however, and that might break some tests.
> 
> I looked at this some more.
> 
> No gdbtk tests test any command for the wrong number of arguments.  So
> we're free to change this however we want.
> 
> Some of the code in gdbtk-cmds.c is quite bad.  For instance there are
> functions which do no checking, or incorrect checking, of the number
> of arguments they are given.
> 
> I discovered that if I just use Tcl_WrongNumArgs and return TCL_ERROR,
> I don't get the right result, because you also have to do this:
> 
>       result_ptr->flags |= GDBTK_IN_TCL_RESULT;
> 
> Note that I changed all the functions in gdbtk-cmds.c that did any
> argument number checking to use Tcl_WrongNumArgs.  However this issue
> has to be resolved first :-(.  I didn't bother fixing all the
> commands, since my time on Insight is limited and I doubt this sort of
> cleanup is very high priority.
> 
> If we really do have to set result_ptr->flags, that's fine --
> but I'd like to add a macro like this:
> 
> #define RETURN_TCL_ERROR \
>     result_ptr->flags |= GDBTK_IN_TCL_RESULT; \
>     return TCL_ERROR
> 
> Then we can just use `RETURN_TCL_ERROR;' all over.  This is ugly, but
> imho more maintainable than remembering to put an assignment
> everywhere we return TCL_ERROR.
> 
> OTOH I don't understand this comment in call_wrapper:
> 
>   /*
>    * Now copy the result over to the true Tcl result.  If GDBTK_TO_RESULT flag
>    * bit is set , this just copies a null object over to the Tcl result,
>    * which is fine because we should reset the result in this case anyway.
>    */
> 
> First, the comment seems to lie.  It mentions GDBTK_TO_RESULT, which
> makes sense, but the code actually checks for GDBTK_IN_TCL_RESULT.
> 
> Ok, I just read the comments in gdbtk.h and it makes a bit more
> sense.  But wouldn't it be easier to adopt the heuristic that if a
> wrapped command returns TCL_ERROR then we should assume that the
> interpreter's result is already correctly set?  Are there cases where
> we explicitly (as opposed to via error()) return TCL_ERROR but rely on
> the call wrapper to set the result?  That seems strange.
> 
> Tom