Re: [PATCH] command trace / source verbose mode

[Daniel Jacobowitz <drow at false dot org>]

Hi Andrew,

Sorry for not getting back to you on this.  Never made it back here
when reviewing patches in reverse.  Trying in patch tracker order
today.

On Thu, Nov 17, 2005 at 03:04:39PM +0000, Andrew STUBBS wrote:
> +/* Command tracing state.  */
> +
> +int source_verbose = 0;
> +int commandtrace = 0;

You've got two of these, but you always check them together.  One
variable and incrementing/decrementing the trace level around source
would work too, right?

> +  char *minusv=NULL;

Spaces around operators please.

> +  int old_source_verbose = source_verbose;

You do this in a couple places, at least here and for control_level.
Given the way GDB handles errors with longjmp, this isn't safe.  I
recommend you use cleanups instead.  Either one to decrement
commandtrace (trace_commands?) and one to decrement control_level,
or write a general function in utils.c to make a cleanup which
decrements an integer.

> +  /* Is there a '-v' in the string somewhere?  */
> +  if (args && (minusv = strstr(args,"-v")))

Is there any benefit to supporting this at the end?  We've already got
some other commands that are strictly command [options] [args], I
think, or at least we do in MI; I would recommend following the same
model here.  If it starts with -v it's an option.

> +  add_setshow_boolean_cmd ("commandtrace", no_class, &commandtrace, _("\

I don't feel too strongly about this, just personal bias, but how about
something other than run-together words for this?  We have a lot of
those in the existing code e.g. remotetimeout, but we've been trying to
either use hyphens or use spaces and sub-menus lately, I think. 
Something like "set trace-commands".  Or, alternatively "set debug
cli-commands".

>      case continue_control:
> +      if (source_verbose || commandtrace)
> +	printf_unfiltered ("Command issued: loop_continue %s\n", cmd->line);

If we do this, we should also use _() so that "Command issued" can be
translated.  Another alternative would be to use the time-honored shell
syntax for this: prefix with plusses up to the nesting level, like sh
-x.  What do you think?

:REVIEWMAIL:

-- 
Daniel Jacobowitz
CodeSourcery