Re: [PATCH] MI: new timing command

[Vladimir Prus <ghost@cs.msu.su>]

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