Re: [PATCH] Fix usage of to_string() for pretty-printers with children

[Phil Muldoon <pmuldoon@redhat.com>]

On 25/05/17 03:33, Peter Linss wrote:

 
 
> gdb/ChangeLog:
> 
> 	* varobj.c (varobj_value_get_print_value): Call pretty-printer
> 	to_string method for value if present even when children 
> 	method is available.
> 	(dynamic_varobj_has_child_method) Remove unused function.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo  (Variable Objects, Result): Update description of
> 	value to reflect to_string output.

Thanks.

>  
> -#if HAVE_PYTHON
> -
> -static int
> -dynamic_varobj_has_child_method (const struct varobj *var)
> -{
> -  PyObject *printer = var->dynamic->pretty_printer;
> -
> -  if (!gdb_python_initialized)
> -    return 0;
> -
> -  gdbpy_enter_varobj enter_py (var);
> -  return PyObject_HasAttr (printer, gdbpy_children_cst);
> -}
> -#endif
> -

In removing the above you are removing the Python environment call
which, among other things, ensures the state of the Python GIL. The
replacement hunk below does not make the same call? Is this
intentional?


>  /* A factory for creating dynamic varobj's iterators.  Returns an
>     iterator object suitable for iterating over VAR's children.  */
>  
> @@ -2420,11 +2405,6 @@ varobj_value_get_print_value (struct value *value,
>  
>        if (value_formatter)
>  	{
> -	  /* First check to see if we have any children at all.  If so,
> -	     we simply return {...}.  */
> -	  if (dynamic_varobj_has_child_method (var))
> -	    return "{...}";
> -
>  	  if (PyObject_HasAttr (value_formatter, gdbpy_to_string_cst))
>  	    {
>  	      struct value *replacement;
> @@ -2486,6 +2466,13 @@ varobj_value_get_print_value (struct value *value,
>  	      if (replacement)
>  		value = replacement;
>  	    }
> +	  else
> +	    {
> +	      /* If we don't have to_string but we have children,
> +		 we simply return {...}.  */
> +	      if (PyObject_HasAttr (value_formatter, gdbpy_children_cst))
> +		return "{...}";
> +	    }

You've removed a function but replaced it with an if (...) (and the
associated safety calls). I'm not sure this is the right thing to do.

Also, if the to_string call has changed, it might constitute an API
change.

And, alas, all changes need tests, unless obvious, and the outcome of
this test is not obvious (well to me ;) )

Cheers

Phil