From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25616 invoked by alias); 8 Jul 2006 12:02:37 -0000 Received: (qmail 25602 invoked by uid 22791); 8 Jul 2006 12:02:36 -0000 X-Spam-Check-By: sourceware.org Received: from nitzan.inter.net.il (HELO nitzan.inter.net.il) (192.114.186.20) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 08 Jul 2006 12:02:33 +0000 Received: from HOME-C4E4A596F7 (IGLD-80-230-70-118.inter.net.il [80.230.70.118]) by nitzan.inter.net.il (MOS 3.7.3-GA) with ESMTP id EBC43111 (AUTH halo1); Sat, 8 Jul 2006 15:02:27 +0300 (IDT) Date: Sat, 08 Jul 2006 12:02:00 -0000 Message-Id: From: Eli Zaretskii To: Andrew STUBBS CC: gdb-patches@sources.redhat.com In-reply-to: <44AE8922.7070704@st.com> (message from Andrew STUBBS on Fri, 07 Jul 2006 17:17:38 +0100) Subject: Re: [PATCH] command trace / source verbose mode Reply-to: Eli Zaretskii References: <437B6228.8010103@st.com> <437C9C07.4020707@st.com> <20060706131559.GA18827@nevyn.them.org> <44AD46E2.6020207@st.com> <20060706173315.GA26692@nevyn.them.org> <44AE8922.7070704@st.com> 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/msg00066.txt.bz2 > Date: Fri, 07 Jul 2006 17:17:38 +0100 > From: Andrew STUBBS > > Here is a new version of the patch. Thanks. > (source_command): Move old contents to source_script(). Please don't use foo() to signal that `foo' is a function. That syntax looks like a call to `foo' with no arguments, which is not what you mean. Just source_script or `source_script' is enough. > +/* Return the source_verbose global variable to its previous state > + on exit from the source command, by whatever means. */ > +static void > +source_verbose_cleanup (void *old_value) > +{ > + source_verbose = (int) old_value; > +} > + > +static void > +source_command (char *args, int from_tty) > +{ > + struct cleanup *old_cleanups; > + char *file = args; > + > + old_cleanups = make_cleanup (source_verbose_cleanup, (void *)source_verbose); > + Ouch! Can't we use something cleaner than casting an int to a void pointer and back? Don't we risk here tripping on issues like int vs pointer size, sign extension, etc.? > case continue_control: > + if (source_verbose || trace_commands) > + printf_unfiltered (_("Command issued: loop_continue %s\n"), cmd->line); Perhaps it's better to have a function to print these messages, which you will then call with a single argument--the command to announce. If nothing else, it will reduce the number of messages to translate to a bare minimum. Also, how about making the message more self-explaining, like this: Trace: loop_continue %s\n or source: loop_continue %s\n or even %s: loop_continue %s where the first %s gets replaced by the file from which we read the commands. I find ``Command issued'' too general to make immediately evident what is being reported here. > @@ -1183,7 +1229,8 @@ Commands defined in this way may have up > source_help_text = xstrprintf (_("\ > Read commands from a file named FILE.\n\ > Note that the file \"%s\" is read automatically in this way\n\ > -when gdb is started."), gdbinit); > +when gdb is started.\n\ > +Use -v to see the name of the commands issued."), gdbinit); I think the second sentence of the doc string should come after the third, since it's just a note. Also ``to see the name of the commands issued'' sounds awkward to me. I suggest the following text instead: Optional -v switch causes each command in FILE to be echoed as it is executed. > +If you need to debug user-defined commands or sourced files you may find it > +useful to enable command tracing. In this mode each command will be printed > +as it is executed, prefixed with @samp{Command issued:}. An @cindex entry here would be useful. Imagine yourself trying to look for this command without knowing its name, and think about the words or phrases you'd have in mind to try to locate the description of such a command--these should be the phrases in the @cindex entry. > +If @code{-v}, for verbose mode, is given then each command will > +be displayed as it is executed. The option must be given before > +@var{filename}, and will be interpreted as part of the filename anywhere else. Please try to rewrite this without passive tense. Active tense tends to make the text more concise and clear.