Re: [PATCH 07/12] MI option --available-children-only

[Keith Seitz <keiths at redhat dot com>]

On 02/14/2014 12:44 AM, Yao Qi wrote:
> This patch adds option --available-children-only to three MI commands,
> parse it, and set corresponding field of 'struct varobj_dynamic'.
>
>   V2:
>   - Document varobj_set_available_children_only.
>   - Return "available-children-only" from "-list-features".
>
> gdb:
>
> 2014-02-14  Pedro Alves  <pedro@codesourcery.com>
> 	    Yao Qi  <yao@codesourcery.com>
>
> 	* mi/mi-cmd-var.c (mi_cmd_var_create): Use mi_getopt to parse
> 	options.  Handle "--available-children-only".
> 	(mi_cmd_var_info_num_children): Likewise.
> 	(mi_cmd_var_list_children): Likewise.
> 	* mi/mi-main.c (mi_cmd_list_features): Add
> 	"available-children-only".
> 	* varobj.c (struct varobj_dynamic) <available_children_only>:
> 	New.
> 	(new_variable, new_root_variable): Update declaration.
> 	(varobj_is_dynamic_p): Return true if available-children-only
> 	is true.
> 	(varobj_create): Add new argument "available_children_only".
> 	Callers update.
> 	(varobj_get_num_children): Handle "available_children_only"
> 	(varobj_set_available_children_only): New function.
> 	(varobj_list_children): Handle "available_children_only".
> 	(install_new_value): Likewise.
> 	(varobj_update): Likewise.
> 	(new_variable): Add new argument "available_children_only".
> 	(new_root_variable): Likewise.
> 	(varobj_invalidate_iter): Likewise.
> 	* varobj.h (varobj_create): Update declaration.
> 	(varobj_set_available_children_only): Declare.

In general, I think this looks good. However, I am really bothered by 
adding --available-children-only to -var-info-num-children and 
-var-list-children.

When --available-children-only is passed to these commands, it 
permanently overrides the varobj's current property value. I don't think 
clients would expect that passing this flag should affect future 
invocations of varobj commands. I know *I* wouldn't expect that 
side-effect-type behavior.

I think a cleaner approach would be to remove the 
--available-children-only option from -var-info-num-children and 
-var-list-children and add a new command 
[-var-show-available-children-only (?)] to allow the UI to flip this 
property if it wants to.

The rest of the patch looks okay to me.

Keith