Re: [PATCH: gdb/mi + doco] -var-update

[Daniel Jacobowitz <drow at false dot org>]

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  <nickrob@snap.net.nz>
> 
> 	* 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