From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23859 invoked by alias); 28 May 2008 21:25:14 -0000 Received: (qmail 23834 invoked by uid 22791); 28 May 2008 21:25:14 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 28 May 2008 21:24:53 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id E622898401; Wed, 28 May 2008 21:24:51 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id BFA789830E; Wed, 28 May 2008 21:24:51 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1K1T8d-0002B4-1c; Wed, 28 May 2008 17:24:51 -0400 Date: Thu, 29 May 2008 01:23:00 -0000 From: Daniel Jacobowitz To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org Subject: Re: [RFC][patch 2/9] export values mechanism to Python Message-ID: <20080528212451.GB2969@caradoc.them.org> Mail-Followup-To: Thiago Jung Bauermann , gdb-patches@sourceware.org References: <20080429155212.444237503@br.ibm.com> <20080429155304.466637516@br.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080429155304.466637516@br.ibm.com> User-Agent: Mutt/1.5.17 (2008-05-11) 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: 2008-05/txt/msg00741.txt.bz2 On Tue, Apr 29, 2008 at 12:52:14PM -0300, Thiago Jung Bauermann wrote: > +python-value.o: $(srcdir)/python/value.c $(defs_h) $(exceptions_h) \ > + $(python_internal_h) $(value_h) > + $(CC) -c $(INTERNAL_CFLAGS) $(PYTHON_CFLAGS) \ > + $(srcdir)/python/value.c -o python-value.o A little indentation (extra two spaces?) on that last line. Comments for the new functions / variables. Either in the header or at the definition or both, as you prefer, but there need to be more comments in this code. > + { "make_value_from_int", gdbpy_make_value_from_int, METH_VARARGS, > + "Make a value from int" }, Do we need make_value_from_int or just make_value (that could also handle e.g. a string)? > +static PyObject * valpy_lazy_p (PyObject *self, PyObject *args); > +static PyObject * valpy_fetch_lazy (PyObject *self, PyObject *args); > +static PyObject * valpy_common_print (PyObject *self, PyObject *args); Stray spaces after *. > + > +static PyMethodDef value_object_methods[] = { > + { "dereference", valpy_dereference, METH_NOARGS, "Dereferences the value." }, > + { "get_element", valpy_get_element, METH_VARARGS, > + "Obtains element inside value." }, Would it be helpful or confusing to automatically expose this as [] in Python, __getitem__? > + { "increment", valpy_increment, METH_VARARGS, > + "Increment value by the given amount." }, Since the amount is specified, this is add rather than increment. Same question as above; it could be __add__. > + { "equals", valpy_equal_p, METH_VARARGS, "Compare values." }, Similarly, __eq__. > + { "is_lazy", valpy_lazy_p, METH_NOARGS, > + "Returns True if the value is lazy, False if not." }, > + { "fetch_lazy", valpy_fetch_lazy, METH_NOARGS, > + "Fetches value from inferior memory." }, Do you have an example where these are useful? I think of them as an implementation detail of values. > + { "common_print", valpy_common_print, METH_VARARGS, "Prints > value." }, __str__? And maybe we want __repr__. If any of the options to common_print are useful, we can have "print"; I think that's better than common_print which is an artifact of GDB's history. > + > +/* FIXME: copy paste from varobj.c */ > +static char * > +value_get_print_value (struct value *value) Yes, do fix :-) Does it need to move to a value-related file instead of varobj.c? > +/* A value owned by GDB is in the all_values chain, so it will be freed > + automatically when not needed anymore (i.e., before the current command > + completes). */ > +PyObject * > +gdb_owned_value_to_value_object (struct value *v) > +{ > + value_object *result = PyObject_New (value_object, &value_object_type); > + if (result != NULL) > + { > + result->value = v; > + result->owned_by_gdb = 1; > + /* FIXME: should we do it? What is it? */ > + /* I don't think it is needed, since a GDB owned value has a very short > + lifetime. The purpose of the list is explained in the comment above > + its declaration. -- bauermann */ > + value_prepend_to_list (&values_in_python, v); > + } > + return (PyObject *)result; > +} What is this function used for in later patches? It seems dangerous to me. GDB is going to discard the value and Python might still have a reference to it after it's free'd. If this is really necessary, we can reference-count values. -- Daniel Jacobowitz CodeSourcery