From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25563 invoked by alias); 4 Apr 2009 16:39:57 -0000 Received: (qmail 25553 invoked by uid 22791); 4 Apr 2009 16:39:55 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from e24smtp05.br.ibm.com (HELO e24smtp05.br.ibm.com) (32.104.18.26) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 04 Apr 2009 16:39:48 +0000 Received: from mailhub1.br.ibm.com (mailhub1.br.ibm.com [9.18.232.109]) by e24smtp05.br.ibm.com (8.13.1/8.13.1) with ESMTP id n34GZpN8003545 for ; Sat, 4 Apr 2009 13:35:51 -0300 Received: from d24av01.br.ibm.com (d24av01.br.ibm.com [9.18.232.46]) by mailhub1.br.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n34GdxZS1437884 for ; Sat, 4 Apr 2009 13:39:59 -0300 Received: from d24av01.br.ibm.com (loopback [127.0.0.1]) by d24av01.br.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n34Gde3a017344 for ; Sat, 4 Apr 2009 13:39:40 -0300 Received: from [9.8.7.95] ([9.8.7.95]) by d24av01.br.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id n34Gde4j017338; Sat, 4 Apr 2009 13:39:40 -0300 Subject: Re: Python pretty-printing [5/6] From: Thiago Jung Bauermann To: Tom Tromey Cc: gdb-patches ml In-Reply-To: References: Content-Type: text/plain; charset=utf-8 Date: Sat, 04 Apr 2009 16:39:00 -0000 Message-Id: <1238863179.3236.134.camel@localhost.localdomain> Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-04/txt/msg00094.txt.bz2 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