From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11074 invoked by alias); 16 Sep 2011 10:45:49 -0000 Received: (qmail 11059 invoked by uid 22791); 16 Sep 2011 10:45:47 -0000 X-SWARE-Spam-Status: No, hits=-6.7 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; Fri, 16 Sep 2011 10:45:31 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p8GAjUYF012601 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 16 Sep 2011 06:45:30 -0400 Received: from localhost.localdomain (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p8GAjSoB028382; Fri, 16 Sep 2011 06:45:29 -0400 From: Phil Muldoon To: Paul Koning Cc: gdb-patches@sourceware.org Subject: Re: Python: add field access by name and standard python mapping methods to gdb.Type References: Reply-to: pmuldoon@redhat.com X-URL: http://www.redhat.com Date: Fri, 16 Sep 2011 10:47:00 -0000 In-Reply-To: (Paul Koning's message of "Thu, 15 Sep 2011 16:38:33 -0400") 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-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: 2011-09/txt/msg00296.txt.bz2 Paul Koning writes: > The attached proposed patch adds access to struct and enum fields by their name, just as field names can be used on gdb.Value objects. It also adds the standard Python methods for mappings (dictionary-like objects), so that the kind of things that Python programmers would expect to work on objects that can be indexed by name indeed do work. For example, you can iterate through the fields, or query if a field name exists. > > For a change like this that adds a new feature which uses a whole pile > of new functions, is the sort of ChangeLog entry shown below the right > way to document the change, or should I write this in a different way? Other than one small nit, your ChangeLog is perfectly acceptable. I think you should on the py-type.c line, list the functions first, then the comment. > > -/* Return a sequence of all fields. Each field is a dictionary with > - some pre-defined keys. */ > +/* Helper function to return the name of a field, as a Python object. > */ "Or None." to document what happens if a name cannot be found. > static PyObject * > -typy_fields (PyObject *self, PyObject *args) > +field_name (struct type *type, int field) > { > PyObject *result; > + > + if (TYPE_FIELD_NAME (type, field)) > + result = PyString_FromString (TYPE_FIELD_NAME (type, field)); > + else > + { > + result = Py_None; > + Py_INCREF (result); > + } > + return result; > +} > +static PyObject * > +make_fielditem (struct type *type, int i, enum gdbpy_iter_kind kind) > +{ > + PyObject *item = NULL, *key = NULL, *value = NULL; > + > + switch (kind) > + { > + case iter_items: > + key = field_name (type, i); > + if (key == NULL) > + goto fail; > + value = convert_field (type, i); > + if (value == NULL) > + goto fail; > + item = PyTuple_New (2); > + if (item == NULL) > + goto fail; > + PyTuple_SET_ITEM (item, 0, key); > + PyTuple_SET_ITEM (item, 1, value); > + break; > + case iter_keys: > + item = field_name (type, i); > + break; > + case iter_values: > + item = convert_field (type, i); > + break; > + } > + return item; > + > + fail: > + Py_XDECREF (key); > + Py_XDECREF (value); > + Py_XDECREF (item); > + return NULL; > +} Because this function returns a value that is not simple (it returns a tuple) a brief comment on the return type and the contents would be great. > +typy_getitem (PyObject *self, PyObject *key) > +{ > + struct type *type = ((type_object *) self)->type; > + char *field; > + int i; > + > + field = python_string_to_host_string (key); > + if (field == NULL) > + return NULL; > + > + /* We want just fields of this type, not of base types, so instead of > + using lookup_struct_elt_type, portions of that function are > + copied here. */ > + > + for (;;) > + { > + CHECK_TYPEDEF (type); > + if (TYPE_CODE (type) != TYPE_CODE_PTR > + && TYPE_CODE (type) != TYPE_CODE_REF) > + break; > + type = TYPE_TARGET_TYPE (type); > + } This gives me pause, not because it is wrong, but because I wonder if there is a possibility that this loop will never exit. I presume it will eventually find the base_type, just by continually walking the TARGET_TYPE until it reaches bottom. Can you check how this is done in other parts of GDB (this must happen quite often?). > +typy_get (PyObject *self, PyObject *args) > +{ > + PyObject *key, *defval = Py_None, *result; > + > + if (!PyArg_UnpackTuple (args, "get", 1, 2, &key, &defval)) > + return NULL; > + > + result = typy_getitem (self, key); > + if (result != NULL) > + return result; > + > + /* getitem returned error, clear the exception status and > + return the defval instead. */ > + PyErr_Clear (); Given that typy_getitem can raise errors that do not relate directly to a lookup (the string conversion call in there might indicate some other conditions, like out of memory, for example), should we only clear exceptions in some specific case? > +/* Implement the "has_key" method on the type object. */ > +static PyObject * > +typy_has_key (PyObject *self, PyObject *args) > +{ > + struct type *type = ((type_object *) self)->type; > + char *field; > + int i; > + > + if (!PyArg_ParseTuple (args, "s", &field)) > + return NULL; > + > + /* We want just fields of this type, not of base types, so instead of > + using lookup_struct_elt_type, portions of that function are > + copied here. */ > + > + for (;;) > + { > + CHECK_TYPEDEF (type); > + if (TYPE_CODE (type) != TYPE_CODE_PTR > + && TYPE_CODE (type) != TYPE_CODE_REF) > + break; > + type = TYPE_TARGET_TYPE (type); > + } See above for the comment on this loop. If GDB does not have a helper macro to this already, you might consider just adding one. Thanks for this patch, I think it is very useful Phil