This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC][patch 2/9] export values mechanism to Python
- From: Daniel Jacobowitz <drow at false dot org>
- To: Thiago Jung Bauermann <bauerman at br dot ibm dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 28 May 2008 17:24:51 -0400
- Subject: Re: [RFC][patch 2/9] export values mechanism to Python
- References: <20080429155212.444237503@br.ibm.com> <20080429155304.466637516@br.ibm.com>
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