Re: [PATCH] MI: new timing command

Vladimir Prus
2006-12-31T15:33:30Z

Nick Roberts wrote:

> > Do you have a *strong* preference to context diff as opposed to
> > unified? If yes, I think I can try to cope with context diffs, but
> > unified diffs are much more readable.
>
> I always use context diffs because that's what Richard Stallman insists on
> for
> Emacs. However, it's no big deal because if you save the patch as a file
> and view it in Emacs in diff-mode (certain names like timings1.diff will
> automatically open in this mode), you can toggle between context and
> unified diffs from the menu bar.

That requires me to (1) save the message (2) open it in Emacs (3) copy-paste
it back to mailer when I have something to say about a patch. That's why I've
asked if you have *strong* preference for context diffs for gdb/MI patches.

> > More seriously, this comment only says what this variable is
> > used for, not what it is. For example, the comment might read:
> >
> > /* The timestamp of the moment when the current
> > command started executing. Used to ... */
> >
> > Ah, and looking at the code this variable is used *only* so that
> > 'run' and friend can output the timestamp despite the fact that
> > they don't emit '^done', so I think this is better
> > represented in the comment.
>
> In Apple's code its used for the asynchronous continuation functions. It's
> of type mi_timestamp and called current_command_ts: I think it's better to
> let the code speak for itself.

Sorry, I think MI has too much code that fails to speak for itself. Do you suggest
that somebody reading this code should grep for current_command_ts to understand
where this value is set?

I also think it's is important to note why this variable exists. Why can't we pass
mi_parse* to the relevant function using function parameters? Is there a fundamental
reason to use global variable?

> >...
> > > + Â  Â  Â  if (do_timings)
> > > + Â Â Â Â Â Â current_command_ts = context->cmd_start;
> >
> > I wonder if it's better, instead of having global current_command_ts,
> > add new global
> >
> > struct mi_parse* current_context;
> >
> > set it here to context:
> >
> > current_context = context;
> >
> > And the user 'current_context' later. That seems to be a more
> > future-proof solution -- if mi_execute_async_cli_command will
> > later need something more from mi_parse structure, we won't
> > need to add yet another global variable.
>
> This is a good solution if mi_execute_async_cli_command does eventually
> need something more from mi_parse structure later but I don't see why it's
> needed
> now. But perhaps you have something in mind?

Using current_context does not make the patch more complex, and gives us some
future proofing. In fact, I'd say the right solution is to add mi_parse* parameter
to all functions down to mi_executed_async_cli_command, but that's solution is
more complex.

Although, if we're going to have "async commands", using a single global variable
will probably not work.

- Volodya