Re: [PATCH] MI: new timing command

[Nick Roberts <nickrob at snap dot net dot nz>]

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


 > > + /* This is used to pass the current command timestamp
 > > + ? ?down to continuation routines. */
 > 
 > Two spaces after dot. No, I personally don't think this coding style
 > guideline makes any sense, but Dan will notice it anyway and you'll
 > have to change ;-)

OK.

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

 >...
 > > + ? ? ? if (strcmp (argv[0], "yes") == 0)
 > > + ??????do_timings = 1;
 > > + ? ? ? else if (strcmp (argv[0], "no") == 0)
 > > + ??????do_timings = 0;
 > > + ? ? ? else
 > > + ??????goto usage_error;
 > 
 > Something looks wrong with indentation above.

It's just the way tabs are handled by the diff.

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

 >...
 > > ? ? ? ? args->rc = mi_cmd_execute (context);
 > > ? 
 > > + ? ? ? if (do_timings)
 > > + ? ? ? ? ? timestamp (&cmd_finished);
 > > + 
 > > ? ? ? ? if (!target_can_async_p () || !target_executing)
 > > ? ??????{
 > > ? ?????? ?/* print the result if there were no errors
 > > *************** captured_mi_execute_command (struct ui_o
 > > *** 1068,1073 ****
 > > --- 1118,1127 ----
 > > ? ?????? ? ? ?fputs_unfiltered ("^done", raw_stdout);
 > > ? ?????? ? ? ?mi_out_put (uiout, raw_stdout);
 > > ? ?????? ? ? ?mi_out_rewind (uiout);
 > > + ?????? ? ? ?/* Have to check cmd_start, since the command could be
 > > + ?????????????? "mi-enable-timings". */
 > 
 > Haven't you named the command 'enable-timings' without 'mi-'?

Duh! Yes.  Apple call it -mi-enable-timings but the mi seems implicit to me.

 >...
 > > + static long 
 > > + wallclock_diff (struct mi_timestamp *start, struct mi_timestamp *end)
 > > + ? {
 > > + ? ? return ((end->wallclock.tv_sec - start->wallclock.tv_sec) * 1000000) +
 > > + ? ? ? ? ? ?(end->wallclock.tv_usec - start->wallclock.tv_usec);
 > > + ? }
 > > + 
 > > + static long 
 > > + user_diff (struct mi_timestamp *start, struct mi_timestamp *end)
 > > + ? {
 > > + ? ? return 
 > > + ? ? ?((end->rusage.ru_utime.tv_sec - start->rusage.ru_utime.tv_sec) * 1000000) +
 > > + ? ? ? (end->rusage.ru_utime.tv_usec - start->rusage.ru_utime.tv_usec);
 > > + ? }
 > > + 
 > > + static long 
 > > + system_diff (struct mi_timestamp *start, struct mi_timestamp *end)
 > > + ? {
 > > + ? ? return 
 > > + ? ? ?((end->rusage.ru_stime.tv_sec - start->rusage.ru_stime.tv_sec) * 1000000) +
 > > + ? ? ? (end->rusage.ru_stime.tv_usec - start->rusage.ru_stime.tv_usec);
 > > + ? }
 > 
 > Perhaps the last three functions can be replaced with
 > 
 > static long
 > timeval_diff (struct timeval* start, start timeval *end)
 > {
 >         return (end->tv_sec - start->tv_sec) * 1000000 .....
 > }
 > 
 > That 'user_diff' seems rather low level on non-reusable.
 > 
 > > + /* Timestamps for current command and last asynchronous command */

Sounds reasonable.  I'll do this in my next patch.

 > Dot at the end of the sentence. The above sounds like this
 > structure hold two separate timestaps -- for current command
 > and the last async command. Maybe replacing "and" with "or" will help.

There are *many* instances of one line comments in this file without a full
stop.  Perhaps the practice is just to give multi-line comments a full stop
(maybe one liners are regarded as phrases).

-- 
Nick                                           http://www.inet.net.nz/~nickrob