From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20845 invoked by alias); 31 Dec 2006 15:33:30 -0000 Received: (qmail 20835 invoked by uid 22791); 31 Dec 2006 15:33:29 -0000 X-Spam-Check-By: sourceware.org Received: from zigzag.lvk.cs.msu.su (HELO zigzag.lvk.cs.msu.su) (158.250.17.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sun, 31 Dec 2006 15:33:25 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.50) id 1H12gW-0002fE-MF for gdb-patches@sources.redhat.com; Sun, 31 Dec 2006 18:33:21 +0300 Received: from localhost ([127.0.0.1] helo=ip6-localhost) by zigzag.lvk.cs.msu.su with esmtp (Exim 4.50) id 1H12gM-0002Zf-1q; Sun, 31 Dec 2006 18:33:06 +0300 From: Vladimir Prus Subject: Re: [PATCH] MI: new timing command To: Nick Roberts , gdb-patches@sources.redhat.com Date: Sun, 31 Dec 2006 15:33:00 -0000 References: <17814.10139.269708.848818@kahikatea.snap.net.nz> <17815.23981.229049.700927@kahikatea.snap.net.nz> User-Agent: KNode/0.10.2 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8Bit Message-Id: 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/msg00410.txt.bz2 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