This is the mail archive of the
insight@sources.redhat.com
mailing list for the Insight project.
Re: Patch: fixlet in gdbtk-cmds.c
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