This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] MI: new timing command
> 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