From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10046 invoked by alias); 31 Dec 2006 06:55:18 -0000 Received: (qmail 10024 invoked by uid 22791); 31 Dec 2006 06:55:17 -0000 X-Spam-Check-By: sourceware.org Received: from viper.snap.net.nz (HELO viper.snap.net.nz) (202.37.101.8) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 31 Dec 2006 06:55:08 +0000 Received: from kahikatea.snap.net.nz (p202-124-124-107.snap.net.nz [202.124.124.107]) by viper.snap.net.nz (Postfix) with ESMTP id A69693D81D1; Sun, 31 Dec 2006 19:54:59 +1300 (NZDT) Received: by kahikatea.snap.net.nz (Postfix, from userid 500) id 853A7BE447; Sun, 31 Dec 2006 19:50:23 +1300 (NZDT) From: Nick Roberts MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Message-ID: <17815.23981.229049.700927@kahikatea.snap.net.nz> Date: Sun, 31 Dec 2006 06:55:00 -0000 To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: [PATCH] MI: new timing command In-Reply-To: References: <17814.10139.269708.848818@kahikatea.snap.net.nz> X-Mailer: VM 7.19 under Emacs 22.0.92.3 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-12/txt/msg00393.txt.bz2 > 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 a= nd 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 > > + =A0 =A0down to continuation routines. */ >=20 > 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: >=20 > /* The timestamp of the moment when the current > command started executing. Used to ... */ >=20 > 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. >... > > + =A0 =A0 =A0 if (strcmp (argv[0], "yes") =3D=3D 0) > > + =A0=A0=A0=A0=A0=A0do_timings =3D 1; > > + =A0 =A0 =A0 else if (strcmp (argv[0], "no") =3D=3D 0) > > + =A0=A0=A0=A0=A0=A0do_timings =3D 0; > > + =A0 =A0 =A0 else > > + =A0=A0=A0=A0=A0=A0goto usage_error; >=20 > Something looks wrong with indentation above. It's just the way tabs are handled by the diff. >... > > + =A0 =A0 =A0 if (do_timings) > > + =A0=A0=A0=A0=A0=A0current_command_ts =3D context->cmd_start; >=20 > I wonder if it's better, instead of having global current_command_ts, > add new global >=20 > struct mi_parse* current_context; >=20 > set it here to context: >=20 > current_context =3D context; >=20 > 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? >... > > =A0 =A0 =A0 =A0 args->rc =3D mi_cmd_execute (context); > > =A0=20 > > + =A0 =A0 =A0 if (do_timings) > > + =A0 =A0 =A0 =A0 =A0 timestamp (&cmd_finished); > > +=20 > > =A0 =A0 =A0 =A0 if (!target_can_async_p () || !target_executing) > > =A0 =A0=A0=A0=A0=A0=A0{ > > =A0 =A0=A0=A0=A0=A0=A0 =A0/* print the result if there were no errors > > *************** captured_mi_execute_command (struct ui_o > > *** 1068,1073 **** > > --- 1118,1127 ---- > > =A0 =A0=A0=A0=A0=A0=A0 =A0 =A0 =A0fputs_unfiltered ("^done", raw_stdou= t); > > =A0 =A0=A0=A0=A0=A0=A0 =A0 =A0 =A0mi_out_put (uiout, raw_stdout); > > =A0 =A0=A0=A0=A0=A0=A0 =A0 =A0 =A0mi_out_rewind (uiout); > > + =A0=A0=A0=A0=A0=A0 =A0 =A0 =A0/* Have to check cmd_start, since the = command could be > > + =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 "mi-enable-timings". */ >=20 > 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=20 > > + wallclock_diff (struct mi_timestamp *start, struct mi_timestamp *end) > > + =A0 { > > + =A0 =A0 return ((end->wallclock.tv_sec - start->wallclock.tv_sec) * = 1000000) + > > + =A0 =A0 =A0 =A0 =A0 =A0(end->wallclock.tv_usec - start->wallclock.tv= _usec); > > + =A0 } > > +=20 > > + static long=20 > > + user_diff (struct mi_timestamp *start, struct mi_timestamp *end) > > + =A0 { > > + =A0 =A0 return=20 > > + =A0 =A0 =A0((end->rusage.ru_utime.tv_sec - start->rusage.ru_utime.tv= _sec) * 1000000) + > > + =A0 =A0 =A0 (end->rusage.ru_utime.tv_usec - start->rusage.ru_utime.t= v_usec); > > + =A0 } > > +=20 > > + static long=20 > > + system_diff (struct mi_timestamp *start, struct mi_timestamp *end) > > + =A0 { > > + =A0 =A0 return=20 > > + =A0 =A0 =A0((end->rusage.ru_stime.tv_sec - start->rusage.ru_stime.tv= _sec) * 1000000) + > > + =A0 =A0 =A0 (end->rusage.ru_stime.tv_usec - start->rusage.ru_stime.t= v_usec); > > + =A0 } >=20 > Perhaps the last three functions can be replaced with >=20 > static long > timeval_diff (struct timeval* start, start timeval *end) > { > return (end->tv_sec - start->tv_sec) * 1000000 ..... > } >=20 > That 'user_diff' seems rather low level on non-reusable. >=20 > > + /* 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). --=20 Nick http://www.inet.net.nz/~nick= rob