Re: [PATCH] command trace / source verbose mode

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri, 07 Jul 2006 17:17:38 +0100
> From: Andrew STUBBS <andrew.stubbs@st.com>
> 
> Here is a new version of the patch.

Thanks.

> 	(source_command): Move old contents to source_script().

Please don't use foo() to signal that `foo' is a function.  That
syntax looks like a call to `foo' with no arguments, which is not what
you mean.  Just source_script or `source_script' is enough.

> +/* Return the source_verbose global variable to its previous state
> +   on exit from the source command, by whatever means.  */
> +static void
> +source_verbose_cleanup (void *old_value)
> +{
> +  source_verbose = (int) old_value;
> +}
> +
> +static void
> +source_command (char *args, int from_tty)
> +{
> +  struct cleanup *old_cleanups;
> +  char *file = args;
> +
> +  old_cleanups = make_cleanup (source_verbose_cleanup, (void *)source_verbose);
> +

Ouch!  Can't we use something cleaner than casting an int to a void
pointer and back?  Don't we risk here tripping on issues like int vs
pointer size, sign extension, etc.?

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

Perhaps it's better to have a function to print these messages, which
you will then call with a single argument--the command to announce.
If nothing else, it will reduce the number of messages to translate to
a bare minimum.

Also, how about making the message more self-explaining, like this:

      Trace: loop_continue %s\n
or
      source: loop_continue %s\n

or even

      %s: loop_continue %s

where the first %s gets replaced by the file from which we read the
commands.  I find ``Command issued'' too general to make immediately
evident what is being reported here.

> @@ -1183,7 +1229,8 @@ Commands defined in this way may have up
>    source_help_text = xstrprintf (_("\
>  Read commands from a file named FILE.\n\
>  Note that the file \"%s\" is read automatically in this way\n\
> -when gdb is started."), gdbinit);
> +when gdb is started.\n\
> +Use -v to see the name of the commands issued."), gdbinit);

I think the second sentence of the doc string should come after the
third, since it's just a note.

Also ``to see the name of the commands issued'' sounds awkward to me.
I suggest the following text instead:

  Optional -v switch causes each command in FILE to be echoed
  as it is executed.

> +If you need to debug user-defined commands or sourced files you may find it
> +useful to enable command tracing.  In this mode each command will be printed
> +as it is executed, prefixed with @samp{Command issued:}.

An @cindex entry here would be useful.  Imagine yourself trying to
look for this command without knowing its name, and think about the
words or phrases you'd have in mind to try to locate the description
of such a command--these should be the phrases in the @cindex entry.

> +If @code{-v}, for verbose mode, is given then each command will
> +be displayed as it is executed.  The option must be given before
> +@var{filename}, and will be interpreted as part of the filename anywhere else.

Please try to rewrite this without passive tense.  Active tense tends
to make the text more concise and clear.