Re: [PATCH] MI: new timing command

Nick Roberts
2006-12-31T06:55:18Z

> 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