Re: Python pretty-printing [5/6]

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Thiago" == Thiago Jung Bauermann <bauerman@br.ibm.com> writes:

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

Ok, I'll do that.

Thiago> Perhaps we should explain which Python objects can be
Thiago> convertible to a GDB value?

Thanks, I did this too.

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

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

Did it.

Tom> +find_pretty_printer (PyObject *value)

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

I looked again and I don't see how it can return NULL without setting
the exception.  Can you point it out to me?

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

Thanks, I fixed this.

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

I made the minimal change here.  I don't want to refactor this right
now; Phil is in the middle of doing that for the embedded \0 problem.
We can apply his fix separately; the current code is not ideal, due to
this problem, but it is still usable for a variety of printers.

[...]
Thiago> But only because varobj.c uses and expects
Thiago> \0-terminated strings, so IMHO it's better to make the change, and
Thiago> either fix or accomodate varobj.c. WDYT?

I'll make sure Phil looks at the varobj part of this problem as well.
It is less clear to me how MI ought to work with embedded \0 in a
string, so if an MI expert wants to speak up...

Tom> +gdbpy_instantiate_printer (PyObject *cons, PyObject *value)

Thiago> Is it worth having this function, which is in fact just a call to
Thiago> PyObject_CallFunctionObjArgs?

Nope, it was a leftover.  I nuked it.

Tom> +char *
Tom> +gdbpy_get_display_hint (PyObject *printer)

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

Thiago> If you agree it's useful, this method should be renamed to not use the
Thiago> gdbpy_ prefix.

I am leaving this for now.  We use the gdbpy_ prefix inconsistently,
even in CVS gdb, and I would prefer to see a single cleanup if we are
going to adopt a solid naming convention.

I will send an updated patch shortly.

Tom