This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Python pretty-printing [5/6]


El jue, 02-04-2009 a las 14:56 -0600, Tom Tromey escribiÃ:
> This is the main part of the pretty-printing code.

Awesome. Some comments follow. Feel free to mention if you think they
cross the line from useful to nag and pedantic.

>  gdb/python/python.c                             |  518 +++++++++++++++++++++++

The pretty-printing specific code in python.c is enough to warrant it to
go in its own .c file. WDYT of moving it to, say, python-prettyprint.c?

I was meaning to post a RFC patch to the archer list doing the move, but
you submitted the code upstream before I got around to it. :-)

> +the second element is the child's value.  The value can be any Python
> +object which is convertible to a @value{GDBN} value.

Perhaps we should explain which Python objects can be convertible to a
GDB value? Something like:

"... which is convertible to a @value{GDBN} value, e.g.: scalars
(integers, floats, booleans), strings and @code{gdb.Value} objects)."

> +We recommend that you put your core pretty-printers into a versioned
> +python package, and then restrict your auto-loaded code to idempotent

Sorry if this is silly, but: at least to me, it's not immediately clear
what "versioned python package" means. The first thing I think of is a
python package checked into a VCS but since this cannot be what the text
is talking about, my brain raises a parser error. Perhaps rewording this
to "... into a python package whose name includes the library version"
or something like that?

I agree that the  example given right after this paragraph clears the
doubts I had, though, so perhaps this is moot.

> +/* Find the pretty-printing constructor function for TYPE.  If no
> +   pretty-printer exists, return NULL.  If one exists, return a new
> +   reference.  */
> +static PyObject *
> +find_pretty_printer (PyObject *value)

s/TYPE/VALUE/

Also, if this function returns NULL, sometimes a Python exception will
be set, sometimes it won't. Is this intended? If so, this should be
noted.

> +/* Pretty-print a single value, via the printer object PRINTER.  If
> +   the function returns a string, an xmalloc()d copy is returned.
> +   Otherwise, if the function returns a value, a *OUT_VALUE is set to
> +   the value, and NULL is returned.  On error, *OUT_VALUE is set to
> +   NULL and NULL is returned.  */
> +static char *
> +pretty_print_one_value (PyObject *printer, struct value **out_value)
> +{
> +  char *output = NULL;
> +  volatile struct gdb_exception except;
> +
> +  TRY_CATCH (except, RETURN_MASK_ALL)
> +    {
> +      PyObject *result;
> +
> +      result = PyObject_CallMethodObjArgs (printer, gdbpy_to_string_cst, NULL);
> +      if (result)
> +	{
> +	  if (gdbpy_is_string (result))
> +	    output = python_string_to_host_string (result);
> +	  else if (PyObject_TypeCheck (result, &value_object_type))
> +	    {
> +	      /* If we just call convert_value_from_python for this
> +		 type, we won't know who owns the result.  For this
> +		 one case we need to copy the resulting value.  */
> +	      struct value *v = value_object_to_value (result);
> +	      *out_value = value_copy (v);
> +	    }
> +	  else
> +	    *out_value = convert_value_from_python (result);
> +	  Py_DECREF (result);
> +	}

Like I said earlier in the archer mailing list:

The comment about convert_value_from_python above is outdated. Jim
Blandy fixed it to always return a caller-owned result.

Because of this, pretty_print_one_value can now probably just call
convert_value_from_python and return a struct value in all cases and be
done with it. Either this function should be changed to work that way,
or the comment above removed.

Since (as you mentioned on IRC yesterday) the call to
python_string_to_host_string will have to be changed anyway to take into
account the length of the Python string, IMHO it's easier to make this
function always return a struct value. Then in print_string_repr you can
call LA_GET_STRING on it before calling LA_PRINT_STRING.

That will solve the problem of assuming that the string ends with a \0
byte (c_get_string still assumes that strings end with a null
character). Well, almost. value_from_string needs to be changed to
receive a length argument instead of using strlen as it does now, and
convert_value_from_python needs to pass the length obtained from the
python string to it. But that's trivial to do.

>From a cursory look at the varobj side of things (where
pretty_print_one_value is also used), things may get more complicated if
this function is changed. But only because varobj.c uses and expects
\0-terminated strings, so IMHO it's better to make the change, and
either fix or accomodate varobj.c. WDYT?

[ This ended up less like a comment and more like a braindump. I tried
to polish the text, though... ]

> +/* Instantiate a pretty-printer given a constructor, CONS, and a
> +   value, VAL.  Return NULL on error.  Ownership of the object
> +   instance is transferred to the reciever */
> +PyObject *
> +gdbpy_instantiate_printer (PyObject *cons, PyObject *value)
> +{
> +  PyObject *result;
> +  result = PyObject_CallFunctionObjArgs (cons, value, NULL);
> +  return result;
> +}

Is it worth having this function, which is in fact just a call to
PyObject_CallFunctionObjArgs? Also, there's a typo in the comment:
s/reciever/receiver/

> +/* Return the display hint for the object printer, PRINTER.  Return
> +   NULL if there is no display_hint method, or if the method did not
> +   return a string.  On error, print stack trace and return NULL.  On
> +   success, return an xmalloc()d string.  */
> +char *
> +gdbpy_get_display_hint (PyObject *printer)

I use the *py_ prefix for functions that can be directly called from
Python. I think it's a useful hint. I don't think I ever mentioned this
practice though.

If you agree it's useful, this method should be renamed to not use the
gdbpy_ prefix.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]