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

[Daniel Jacobowitz <drow at false dot org>]

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