Re: [PATCH] command trace / source verbose mode

[Andrew STUBBS <andrew dot stubbs at st dot com>]

Daniel Jacobowitz wrote:
> 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.

No problem, but I knew the patch tracker would be a good thing. :)

> 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?

Yeah, that might work too. But the downside is that if the 'set debug 
commandtrace on' is issued multiple times then the user will have to 
turn it off that many times also.

> > +  /* 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.

Does it do any harm beyond making the code a little more complex? I was 
rather expecting you to say something about the fact that it changes the 
argument to the --command option (I documented the change).

> > +  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".

Well, since we have been using this for a long time now (long before I 
posted the patch) I will need to keep this version in our own sources 
for purposes of backwards compatibility. That being so, it really 
doesn't matter to me what official name is chosen.

I think keeping it under 'set debug' is a good idea, but maybe because 
it is for debugging other than GDB itself it should be promoted to 
somewhere more prominent. Hmmm, perhaps "set trace-commands" it the best 
option.

I'll have a think about it, work on the other points you raised, and get 
back to you.

Andrew