[python] [rfc] Patch for pretty-printers registration API change
Tom Tromey
tromey@redhat.com
Wed Feb 18 19:15:00 GMT 2009
Thanks Phil.
Phil> + function = PyList_GetItem (list, list_index);
Phil> + if (function == NULL)
Phil> + Py_RETURN_NONE;
I think it is not valid to ignore Python errors like this. Instead, I
think you either have to propagate the exception (return NULL) or
clear it. I suspect propagation is the better answer.
Phil> + printer = gdbpy_instantiate_printer (function, value);
Phil> + if (printer && (printer != Py_None))
Still too many parens :)
Also, the same comment holds about error propagation.
Phil> + val_obj = value_to_value_object (value);
This can fail, returning NULL. I think this particular line should
probably be moved out of the TRY_CATCH and after the
GDB_PY_HANDLE_EXCEPTION. Then, it needs an error check.
(Though see below, I think some of this is probably redundant.)
Phil> + Py_DECREF (val_obj);
I'm guessing you'll want Py_XDECREF here.
Phil> value = value_from_contents_and_address (type, valaddr, address);
[...]
Phil> + printer = find_pretty_printer (value);
In this case we know we don't need to copy the value -- we made a new
one and we can just use it directly. So, I think the value_copy logic
should probably be removed from find_pretty_printer and pushed into
the callers that need it. What do you think about that?
Phil> + // Do not instantiate NoneType.
Phil> + if (constructor == Py_None)
Phil> + pretty_printer = Py_None;
Phil> + else
Phil> + pretty_printer = instantiate_pretty_printer (constructor, var->value);
Phil> - if (! constructor)
Phil> + if (! pretty_printer)
Phil> {
Phil> gdbpy_print_stack ();
Phil> error ("Could not evaluate visualizer expression: %s", visualizer);
Phil> }
Phil> - if (constructor == Py_None)
Phil> + if (pretty_printer == Py_None)
Phil> {
Phil> - Py_DECREF (constructor);
Phil> - constructor = NULL;
Phil> + Py_DECREF (pretty_printer);
Phil> + pretty_printer = NULL;
Phil> }
The refcount logic for "None" here is not correct. I think the
simplest fix is to add an incref to the true branch of the first if.
Tom
More information about the Archer
mailing list