From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19817 invoked by alias); 6 Jul 2006 13:16:06 -0000 Received: (qmail 19806 invoked by uid 22791); 6 Jul 2006 13:16:06 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Thu, 06 Jul 2006 13:16:04 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1FyTi4-00050L-MY; Thu, 06 Jul 2006 09:16:00 -0400 Date: Thu, 06 Jul 2006 13:16:00 -0000 From: Daniel Jacobowitz To: Andrew STUBBS Cc: Eli Zaretskii , gdb-patches@sources.redhat.com Subject: Re: [PATCH] command trace / source verbose mode Message-ID: <20060706131559.GA18827@nevyn.them.org> Mail-Followup-To: Andrew STUBBS , Eli Zaretskii , gdb-patches@sources.redhat.com References: <437B6228.8010103@st.com> <437C9C07.4020707@st.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <437C9C07.4020707@st.com> User-Agent: Mutt/1.5.11+cvs20060403 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-07/txt/msg00029.txt.bz2 Hi Andrew, Sorry for not getting back to you on this. Never made it back here when reviewing patches in reverse. Trying in patch tracker order today. On Thu, Nov 17, 2005 at 03:04:39PM +0000, Andrew STUBBS wrote: > +/* Command tracing state. */ > + > +int source_verbose = 0; > +int commandtrace = 0; You've got two of these, but you always check them together. One variable and incrementing/decrementing the trace level around source would work too, right? > + char *minusv=NULL; Spaces around operators please. > + int old_source_verbose = source_verbose; You do this in a couple places, at least here and for control_level. Given the way GDB handles errors with longjmp, this isn't safe. I recommend you use cleanups instead. Either one to decrement commandtrace (trace_commands?) and one to decrement control_level, or write a general function in utils.c to make a cleanup which decrements an integer. > + /* Is there a '-v' in the string somewhere? */ > + if (args && (minusv = strstr(args,"-v"))) Is there any benefit to supporting this at the end? We've already got some other commands that are strictly command [options] [args], I think, or at least we do in MI; I would recommend following the same model here. If it starts with -v it's an option. > + add_setshow_boolean_cmd ("commandtrace", no_class, &commandtrace, _("\ I don't feel too strongly about this, just personal bias, but how about something other than run-together words for this? We have a lot of those in the existing code e.g. remotetimeout, but we've been trying to either use hyphens or use spaces and sub-menus lately, I think. Something like "set trace-commands". Or, alternatively "set debug cli-commands". > case continue_control: > + if (source_verbose || commandtrace) > + printf_unfiltered ("Command issued: loop_continue %s\n", cmd->line); If we do this, we should also use _() so that "Command issued" can be translated. Another alternative would be to use the time-honored shell syntax for this: prefix with plusses up to the nesting level, like sh -x. What do you think? :REVIEWMAIL: -- Daniel Jacobowitz CodeSourcery