Re: Patch: fixlet in gdbtk-cmds.c

[Tom Tromey <tromey at cygnus dot com>]

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