Re: [PATCH] Allow nested python pretty printers.

Phil Muldoon
2011-08-17T09:56:47Z

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