Re: [PATCH] Allow nested python pretty printers.

[Phil Muldoon <pmuldoon at redhat dot com>]

andrew@ado.is-a-geek.net writes:

> From: Andrew Oakley <andrew@ado.is-a-geek.net>
>
> I don't think this is quite ready to commit - MI support needs to be added and
> documentation needs to be updated.  I thought it was sensible to post it now
> though so I can deal with any feedback and so that others don't find the need
> to implement the same thing again.

Understood.  If you want comments on a work-in-progress, (I believe) you
should submit it to gdb-patches anyway, with a [RFC] tag.


> By allowing the children iterator of pretty printers to return more
> pretty printers multi levels of display can be created.

I think this idea is ok, but I do not think renaming functions ad-hoc is
ok.

> - try_convert_value_from_python trys to convert a python PyObject to a
>   gdb struct value like convert_value_from_python but does not raise an
>   error if the PyObject type was not valid.

I think if you want to do this, you should have
try_convert_value_from_python wrap convert_value_from_python and deal
with the exception internally, instead of renaming the function.

> - run_pretty_printer performs all of the steps to print a value given a

The name change seems gratuitous.

> -/* Helper for apply_val_pretty_printer that formats children of the
> -   printer, if any exist.  If is_py_none is true, then nothing has
> -   been printed by to_string, and format output accordingly. */
> +/* Helper for apply_val_pretty_printer that runs a pretty printer,
> +   including formattin of the children if any exist.  */

Typo formattin.

>  static void
> -print_children (PyObject *printer, const char *hint,
> -		struct ui_file *stream, int recurse,
> -		const struct value_print_options *options,
> -		const struct language_defn *language,
> -		int is_py_none)
> +run_pretty_printer (PyObject *printer, struct ui_file *stream, int recurse,
> +		    const struct value_print_options *options,
> +		    const struct language_defn *language,
> +		    struct gdbarch *gdbarch)

I'm not sure I agree with name change.  I don't want to bike-shed this
but "print_children" is a legitimate atomic operation that should remain
as its own function.  I would suggest architecting another function to
call the relevant pieces if we detect nested printers.

>    if (!iter)
> @@ -634,15 +645,33 @@ print_children (PyObject *printer, const char *hint,
>  	}
>        else
>  	{
> -	  struct value *value = convert_value_from_python (py_v);
> +	  struct value *value;
>  
> -	  if (value == NULL)
> +	  if (try_convert_value_from_python (py_v, &value))
> +	    {
> +	      if (value == NULL)
> +		{
> +		  gdbpy_print_stack ();
> +		  error (_("Error while executing Python code."));
> +		}

I'm not sure what his change  gives us?  You still raise an exception,
if value = NULL? Which is what the old code did.

> +	      else
> +		common_val_print (value, stream, recurse + 1, options,
> +				  language);
> +	    }
> +	  else if (PyObject_HasAttr (py_v, gdbpy_to_string_cst))
> +	    {
> +	      /* have another pretty printer to run */
> +	      run_pretty_printer (py_v, stream, recurse + 1, options, language,
> +	      			  gdbarch);
> +	    }

So, in this case, if try_convert_value_to_python returns false, one
assumes there is a nested printer?  Can printers nest other printer
ad infinitum? What happens if the nested printer returns another nested
printer? It's not clear here what the implications are.


>  /* Try to convert a Python value to a gdb value.  If the value cannot
> -   be converted, set a Python exception and return NULL.  Returns a
> -   reference to a new value on the all_values chain.  */
> -
> -struct value *
> -convert_value_from_python (PyObject *obj)
> +   be converted because it was of the incorrect type return 0.  If the value
> +   was of the correct type set value to a reference to a new value on the
> +   all_values chain and returns 1.  If a Python exception occurs attempting to
> +   convert the value then value is set to NULL and 1 is returned */
> +int
> +try_convert_value_from_python (PyObject *obj, struct value **value)
>  {
> -  struct value *value = NULL; /* -Wall */
> +  int typeok = 1;

As I noted, I really don't like this change.  I think you should create
a new function that wraps this one and discards the exception rather
than altering this.  Essentially the opposite of what you have done.


> +/* Try to convert a Python value to a gdb value.  If the value cannot
> +   be converted, set a Python exception and return NULL.  Returns a
> +   reference to a new value on the all_values chain.  */
> +struct value *
> +convert_value_from_python (PyObject *obj)
> +{
> +  struct value * value;
> +  if (!try_convert_value_from_python(obj, &value))
> +    PyErr_Format (PyExc_TypeError,
> +		  _("Could not convert Python object: %s."),
> +		  PyString_AsString (PyObject_Str (obj)));
>    return value;


So this would be try_convert_value_from_python  that just discarded the
exception.  But there are deeper problems.  In this case, if there
really is an error converting a value, in your code you replace it with
a generic exception.  I think the old code is wrong in this too.  I
think if there is an exception, we should not mask it.  I guess we could
mask particular types given your case.

Cheers,

Phil