Re: [RFC][patch 2/9] export values mechanism to Python

Daniel Jacobowitz
2008-05-28T21:25:14Z

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