From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19732 invoked by alias); 17 Aug 2011 09:56:47 -0000 Received: (qmail 19722 invoked by uid 22791); 17 Aug 2011 09:56:45 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 17 Aug 2011 09:56:29 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p7H9uPFq002899 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 17 Aug 2011 05:56:26 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p7H9uO49001424; Wed, 17 Aug 2011 05:56:25 -0400 From: Phil Muldoon To: andrew@ado.is-a-geek.net Cc: gdb@sourceware.org Subject: Re: [PATCH] Allow nested python pretty printers. References: <1313533386-3222-1-git-send-email-andrew@ado.is-a-geek.net> Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Wed, 17 Aug 2011 09:56:00 -0000 In-Reply-To: <1313533386-3222-1-git-send-email-andrew@ado.is-a-geek.net> (andrew@ado.is-a-geek.net's message of "Tue, 16 Aug 2011 23:23:06 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gdb-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00066.txt.bz2 andrew@ado.is-a-geek.net writes: > From: Andrew Oakley > > I don't think this is quite ready to commit - MI support needs to be added and > documentation needs to be updated. I thought it was sensible to post it now > though so I can deal with any feedback and so that others don't find the need > to implement the same thing again. Understood. If you want comments on a work-in-progress, (I believe) you should submit it to gdb-patches anyway, with a [RFC] tag. > By allowing the children iterator of pretty printers to return more > pretty printers multi levels of display can be created. I think this idea is ok, but I do not think renaming functions ad-hoc is ok. > - try_convert_value_from_python trys to convert a python PyObject to a > gdb struct value like convert_value_from_python but does not raise an > error if the PyObject type was not valid. I think if you want to do this, you should have try_convert_value_from_python wrap convert_value_from_python and deal with the exception internally, instead of renaming the function. > - run_pretty_printer performs all of the steps to print a value given a The name change seems gratuitous. > -/* Helper for apply_val_pretty_printer that formats children of the > - printer, if any exist. If is_py_none is true, then nothing has > - been printed by to_string, and format output accordingly. */ > +/* Helper for apply_val_pretty_printer that runs a pretty printer, > + including formattin of the children if any exist. */ Typo formattin. > static void > -print_children (PyObject *printer, const char *hint, > - struct ui_file *stream, int recurse, > - const struct value_print_options *options, > - const struct language_defn *language, > - int is_py_none) > +run_pretty_printer (PyObject *printer, struct ui_file *stream, int recurse, > + const struct value_print_options *options, > + const struct language_defn *language, > + struct gdbarch *gdbarch) I'm not sure I agree with name change. I don't want to bike-shed this but "print_children" is a legitimate atomic operation that should remain as its own function. I would suggest architecting another function to call the relevant pieces if we detect nested printers. > if (!iter) > @@ -634,15 +645,33 @@ print_children (PyObject *printer, const char *hint, > } > else > { > - struct value *value = convert_value_from_python (py_v); > + struct value *value; > > - if (value == NULL) > + if (try_convert_value_from_python (py_v, &value)) > + { > + if (value == NULL) > + { > + gdbpy_print_stack (); > + error (_("Error while executing Python code.")); > + } I'm not sure what his change gives us? You still raise an exception, if value = NULL? Which is what the old code did. > + else > + common_val_print (value, stream, recurse + 1, options, > + language); > + } > + else if (PyObject_HasAttr (py_v, gdbpy_to_string_cst)) > + { > + /* have another pretty printer to run */ > + run_pretty_printer (py_v, stream, recurse + 1, options, language, > + gdbarch); > + } So, in this case, if try_convert_value_to_python returns false, one assumes there is a nested printer? Can printers nest other printer ad infinitum? What happens if the nested printer returns another nested printer? It's not clear here what the implications are. > /* Try to convert a Python value to a gdb value. If the value cannot > - be converted, set a Python exception and return NULL. Returns a > - reference to a new value on the all_values chain. */ > - > -struct value * > -convert_value_from_python (PyObject *obj) > + be converted because it was of the incorrect type return 0. If the value > + was of the correct type set value to a reference to a new value on the > + all_values chain and returns 1. If a Python exception occurs attempting to > + convert the value then value is set to NULL and 1 is returned */ > +int > +try_convert_value_from_python (PyObject *obj, struct value **value) > { > - struct value *value = NULL; /* -Wall */ > + int typeok = 1; As I noted, I really don't like this change. I think you should create a new function that wraps this one and discards the exception rather than altering this. Essentially the opposite of what you have done. > +/* Try to convert a Python value to a gdb value. If the value cannot > + be converted, set a Python exception and return NULL. Returns a > + reference to a new value on the all_values chain. */ > +struct value * > +convert_value_from_python (PyObject *obj) > +{ > + struct value * value; > + if (!try_convert_value_from_python(obj, &value)) > + PyErr_Format (PyExc_TypeError, > + _("Could not convert Python object: %s."), > + PyString_AsString (PyObject_Str (obj))); > return value; So this would be try_convert_value_from_python that just discarded the exception. But there are deeper problems. In this case, if there really is an error converting a value, in your code you replace it with a generic exception. I think the old code is wrong in this too. I think if there is an exception, we should not mask it. I guess we could mask particular types given your case. Cheers, Phil