From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3395 invoked by alias); 17 Jun 2005 03:43:42 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 3374 invoked by uid 22791); 17 Jun 2005 03:43:33 -0000 Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Fri, 17 Jun 2005 03:43:33 +0000 Received: from drow by nevyn.them.org with local (Exim 4.51) id 1Dj7lR-0001rM-HS; Thu, 16 Jun 2005 23:43:29 -0400 Date: Fri, 17 Jun 2005 03:43:00 -0000 From: Daniel Jacobowitz To: Nick Roberts Cc: Bob Rossi , gdb-patches@sources.redhat.com Subject: Re: [PATCH: gdb/mi + doco] -var-update Message-ID: <20050617034329.GH17013@nevyn.them.org> Mail-Followup-To: Nick Roberts , Bob Rossi , gdb-patches@sources.redhat.com References: <16921.18627.457594.938060@farnswood.snap.net.nz> <01c517d0$Blat.v2.4$09a26040@zahav.net.il> <16922.43915.346792.973282@farnswood.snap.net.nz> <01c51898$Blat.v2.4$f6fd05c0@zahav.net.il> <16929.8147.933720.246602@farnswood.snap.net.nz> <16955.41017.161288.832646@farnswood.snap.net.nz> <20050401024942.GA2179@white> <17013.35649.62745.226730@farnswood.snap.net.nz> <20050502040526.GA10023@nevyn.them.org> <17013.54662.20554.239976@farnswood.snap.net.nz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <17013.54662.20554.239976@farnswood.snap.net.nz> User-Agent: Mutt/1.5.8i X-SW-Source: 2005-06/txt/msg00240.txt.bz2 This is basically OK. A couple of things: On Mon, May 02, 2005 at 07:23:50PM +1200, Nick Roberts wrote: > 2005-05-02 Nick Roberts > > * mi/mi-cmds.h: Add extern declarations for string constants for > MI command options. > > * mi/mi-cmd-var.c: Define string constants for MI command options. > (mi_cmd_var_list_children): : Use string constants instead of Stray colon there. > literals for MI command options. > (mi_cmd_var_update): New option --with-values for -var-update. > (varobj_update_one): Add argument for new option. > > * mi/mi-cmd-stack.c (mi_cmd_stack_list_locals): Use string > constants instead of literals for MI command options. > + extern const char mi_no_values[]; > + extern const char mi_with_values[]; > + extern const char mi_simple_values[]; > + extern const char mi_all_values[]; Both here and in mi-cmd-var.c, please list these in the changelog: * mi/mi-cmds.h (mi_no_values, mi_with_values, mi_simple_values) (mi_all_values): New declarations. * mi/mi-cmds.c (mi_no_values, mi_with_values, mi_simple_values) (mi_all_values): New string constants. ... > --- 268,296 ---- > error (_("mi_cmd_var_list_children: Usage: [PRINT_VALUES] NAME")); > > /* Get varobj handle, if a valid var obj name was specified */ > ! if (argc == 1) > ! var = varobj_get_handle (argv[0]); > ! else > ! var = varobj_get_handle (argv[1]); > if (var == NULL) > error (_("Variable object not found")); > > numchild = varobj_list_children (var, &childlist); > ui_out_field_int (uiout, "numchild", numchild); > if (argc == 2) > ! { > ! if (strcmp (argv[0], "0") == 0 > ! || strcmp (argv[0], mi_no_values) == 0) > ! print_values = PRINT_NO_VALUES; > ! else if (strcmp (argv[0], "1") == 0 > ! || strcmp (argv[0], mi_with_values) == 0) > ! print_values = PRINT_ALL_VALUES; > ! else > ! error (_("Unknown value for PRINT_VALUES: \ > ! must be: 0 or \"%s\", 1 or \"%s\""), mi_no_values, mi_with_values); > ! } > ! else > ! print_values = PRINT_NO_VALUES; > > if (numchild <= 0) > return MI_CMD_DONE; Andrew's right that it would be nice to use mi_getopt here, but we can't as-is; it doesn't do long options. That'd be good to fix someday. You've replaced "--all-values" in the source with "--with-values" here. Surely that's a bug? I don't remember the entire outcome of your discussion with Eli, but I find the idea of having --with-values sometimes and --all-values other times a bit confusing. I went trying to figure out which meant what and that's when I noticed this problem. > ! if (argc == 1) > ! name = argv[0]; > ! else > ! name = (argv[1]); Stray parentheses. > ! if (argc == 2) > ! { > ! if (strcmp (argv[0], "0") == 0 > ! || strcmp (argv[0], mi_no_values) == 0) > ! print_values = PRINT_NO_VALUES; > ! else if (strcmp (argv[0], "1") == 0 > ! || strcmp (argv[0], mi_with_values) == 0) > ! print_values = PRINT_ALL_VALUES; > ! else > ! error (_("Unknown value for PRINT_VALUES: \ > ! must be: 0 or \"%s\", 1 or \"%s\""), mi_no_values, mi_with_values); > ! } > ! else > ! print_values = PRINT_NO_VALUES; > > /* Check if the parameter is a "*" which means that we want > to update all variables */ IIRC, you added the "0"/"1" compatibility to -var-list-children to make life easier for Apple. Is that right? If so, do they need it here also, or can we get away with just --all-values? I've no real objection to the 0/1, but they're a bit ugly. > *** /home/nick/src/gdb/doc/gdb.texinfo.~1.249.~ 2005-05-02 14:28:29.000000000 +1200 > --- /home/nick/src/gdb/doc/gdb.texinfo 2005-05-02 19:12:12.000000000 +1200 > *************** > *** 20331,20336 **** > --- 20331,20342 ---- > variables. With an optional preceding argument of 1 or @code{--all-values}, > also prints their values. > > + Returns a list of the children of the specified variable object. With > + just the variable object name as an argument or with an optional > + preceding argument of 0 or @code{--no-values}, prints only the names > + of the variables. With an optional value for @var{print-values} of 1 > + or @code{--with-values}, also prints their values. > + > @subsubheading Example > > @smallexample This paragraph must be in the wrong place. It's mostly duplicating the paragraph above it. > *************** > *** 20451,20463 **** > @subsubheading Synopsis > > @smallexample > ! -var-update @{@var{name} | "*"@} > @end smallexample > > Update the value of the variable object @var{name} by evaluating its > expression after fetching all the new values from memory or registers. > ! A @samp{*} causes all existing variable objects to be updated. > > > @node Annotations > @chapter @value{GDBN} Annotations > --- 20457,20485 ---- > @subsubheading Synopsis > > @smallexample > ! -var-update [@var{print-values}] @{@var{name} | "*"@} > @end smallexample > > Update the value of the variable object @var{name} by evaluating its > expression after fetching all the new values from memory or registers. > ! A @samp{*} causes all existing variable objects to be updated. With > ! just a single argument or with an optional value for > ! @var{print-values} of 0 or @code{--no-values}, prints only the names > ! of the variables. A value for @var{print-values} of 1 or > ! @code{--with-values}, also prints their values. I think the comma probably shouldn't be there. > ! @subsubheading Example > > + @smallexample > + (@value{GDBP}) > + -var-assign var1 3 > + ^done,value="3" > + (@value{GDBP}) > + -var-update --all-values var1 > + ^done,changelist=[@{name="var1",value="3",in_scope="true", > + type_changed="false"@}] > + (@value{GDBP}) > + @end smallexample > > @node Annotations > @chapter @value{GDBN} Annotations > -- Daniel Jacobowitz CodeSourcery, LLC