Re: [python] [rfc] Patch for pretty-printers registration API change

[Tom Tromey <tromey at redhat dot com>]

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